Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle constraints from run_exports #30

Merged
merged 4 commits into from
May 7, 2024
Merged

Conversation

minrk
Copy link
Contributor

@minrk minrk commented May 7, 2024

  • recognize weak_constrains and strong_constrains keys in run_exports
  • explicitly handle run_exports lists and dicts separately, rather than conflating unrecognized dict keys with weak list entries. Now unrecognized keys are ignored with a warning rather than interpreted as weak requirements

I initially added the constraints to the dependencies for the solve, since right now they are ignored, but I'm not sure that's the right thing to do, so I backed it out.

previous handling added any unrecognized keys (e.g. strong_constrains) to weak requirements, resulting in:

Could not solve for environment specs
  The following package could not be installed
  └─ strong_constrains does not exist (perhaps a typo or a missing channel).

seen in the petsc321 migrator in dolfinx_mpc, specifically this run.

previous handling added any unrecognized keys (e.g. strong_constrains) to weak requirements

resulting in "package not found: strong_constrains"
and constraints being ignored
@beckermr
Copy link
Contributor

beckermr commented May 7, 2024

Thanks for this! Edit: we need to add the constrains bits as predicates to the solver.

@jaimergp should be able to guide us on how to do that.

minrk added 2 commits May 7, 2024 15:24
both weak and strong apply from host environment, not just weak
Copy link
Contributor

@beckermr beckermr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @minrk. My initial comment was wrong and we should not be adding constraints to the host or run deps. I will back those commits out.

Copy link
Contributor

@beckermr beckermr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Back these out.

conda_forge_feedstock_check_solvable/mamba_solver.py Outdated Show resolved Hide resolved
conda_forge_feedstock_check_solvable/mamba_solver.py Outdated Show resolved Hide resolved
conda_forge_feedstock_check_solvable/mamba_solver.py Outdated Show resolved Hide resolved
conda_forge_feedstock_check_solvable/mamba_solver.py Outdated Show resolved Hide resolved
conda_forge_feedstock_check_solvable/mamba_solver.py Outdated Show resolved Hide resolved
conda_forge_feedstock_check_solvable/mamba_solver.py Outdated Show resolved Hide resolved
@minrk
Copy link
Contributor Author

minrk commented May 7, 2024

Makes sense, sorry.

I think I see how to apply constraints as actual constraints instead of dependencies, if you want to go that way. I can do it as another PR, since this fixes the immediate problem and nobody has complained about lacking constraints so far, as far as I know.

@jaimergp
Copy link
Collaborator

jaimergp commented May 7, 2024

Sorry, just saw the ping. I guess it's not necessary anymore to discuss the solver constraints?

@beckermr
Copy link
Contributor

beckermr commented May 7, 2024

We want to inject them into the solver @jaimergp so yeah any help is appreciated.

Let's definitely move to another PR @minrk !

@beckermr beckermr merged commit 10099c2 into regro:main May 7, 2024
2 checks passed
@jaimergp
Copy link
Collaborator

jaimergp commented May 7, 2024

Cool, ping me in the new PR once ready :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants