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

Merge Version Constraints from Multiple Input Files #300

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

srilman
Copy link
Contributor

@srilman srilman commented Dec 18, 2022

Adds version merging behavior discussed in #278. Currently, if multiple input files contain the same dependency with different version constraints,conda-lock will use the version constraints from the last file passed in. This PR has conda-lock combine / AND the constraints together.

Thus, if there are a pair of incompatible version constraints from 2 different files (say >=1,<2 and =2), conda-lock will now fail.

EDIT: Due to #312, this will now only combine version constraints applied to the same platform. So something like

- python =3.9 # [win]
- python =3.10 # [linux]

will not be combined together

@netlify
Copy link

netlify bot commented Dec 18, 2022

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit 12bc805
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/63cd9d48d5123e00092d8bac
😎 Deploy Preview https://deploy-preview-300--conda-lock.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@srilman
Copy link
Contributor Author

srilman commented Dec 31, 2022

FYI I am still working on this, but I am running into issues running some of the unit tests. conda-lock seems to be hanging when solving for pip dependencies. This hasn't happened before, so I am a bit lost. Since last working on this PR, I did install poetry on my machine, but I can't imagine how that would cause conflicts.

Planning to use Docker to get a new dev environment set up and make some progress on this.

@maresb
Copy link
Contributor

maresb commented Dec 31, 2022

The tests are unfortunately somewhat flaky. It doesn't necessarily mean that the problem is on your side.

I find the previous implementation of some stuff fairly suspect. For example,

            previous_selectors |= dep.selectors

This looks like it actually modifies the prev_dep object. I can't imagine that this was intended...

Luckily, |= (implemented by Selectors.__ior__) seems to only occur in this one place. It seems to me like we may want to eliminate and reimplement it.

Also, I don't think it even makes sense to merge different selectors. For instance,

- python =3.9 # [win]
- python =3.10 # [linux]

seems like a perfectly reasonable specification which can't be merged. (Or am I just confused?)

Also, we may want to raise an error on mixing URLDependencies and VersionDependencies, or when mixing two URLDependencies which are not equal.

@maresb
Copy link
Contributor

maresb commented Dec 31, 2022

Those preexisting issues aside, it looks really excellent! Thanks so much for putting this together. Thanks for your patience in waiting for review. I hope we can get this in soon.

@srilman
Copy link
Contributor Author

srilman commented Jan 1, 2023

Also, we may want to raise an error on mixing URLDependencies and VersionDependencies, or when mixing two URLDependencies which are not equal.

Good point, will add.

Also, I don't think it even makes sense to merge different selectors. For instance,

- python =3.9 # [win]
- python =3.10 # [linux]

seems like a perfectly reasonable specification which can't be merged. (Or am I just confused?)

No, I think you're correct. They shouldn't be merged together. But what about selectors that partially overlap? For example,

python >=3.7
python <3.11  #[win]

should technically be transformed to something like

python >=3.7,<3.11  #[win]
python >=3.7        #[not win]

How do we want to handle this case?

@srilman
Copy link
Contributor Author

srilman commented Jan 1, 2023

Also, the way conda-lock currently generates lock specs for environment files is by creating a spec per platform and merging them together. For each spec, the selector for every dependency is just the current platform being rendered. So we rely heavily on taking the union of dependencies specs with different selectors.

@maresb
Copy link
Contributor

maresb commented Jan 1, 2023

should technically be transformed to something like

python >=3.7,<3.11  #[win]
python >=3.7        #[not win]

Excellent point, I agree. Although I'm not sure if we want to consider this as an explicit "transformation". Ideally, in my mind, the platforms should somehow be computed independently.

How do we want to handle this case?

Great question. I think it requires much more careful consideration than I can afford tonight. It would be nice to avoid any convoluted constructions. Probably someone will soon want to extend the current functionality of selectors. (For instance, I don't think conda-lock supports not in selectors.) So ideally we shouldn't implement anything that will make that difficult.

@srilman
Copy link
Contributor Author

srilman commented Jan 22, 2023

I rewrote this PR to work on top of #316 and to reuse some helper functions from the conda vendor package.

  • Now, dependency versions will only be merged if they are multiple versions for the same platform. This works due to the new SourceFile approached implemented in the previous PR
  • This uses the treeify and untreeify functions from the conda vendor package to combine conda version strings. This is a lot simpler than parsing code from the previous implementation.

@riccardoporreca
Copy link
Contributor

@maresb, @srilman, linking here a question on the current behavior: #222 (comment)

This is by design. Files specified later are intended to be overrides to the specifications earlier

The change proposed here can be potentially breaking, and I have been in fact leveraging this behavior for a project where I want to provide an additional file overriding specific package versions. Would you consider an opt-in possibility of re-enabling the current behavior?

@maresb
Copy link
Contributor

maresb commented Feb 9, 2023

Thanks @riccardoporreca for bringing this to our attention.

My primary motivating use case is to be able to specify dev dependencies alongside project dependencies, and so I want them to combine rather than override.

I can see that overriding versions is a valid method of combining dependencies, but it seems less motivated to me. Can you explain a little bit more about why you want to override version numbers in a way that's incompatible with the previous spec?

In any case, opting in to avoid a breaking change seems quite sensible. @mariusvniekerk, does it make sense to you to support combining deps, or are we off-track?

@riccardoporreca
Copy link
Contributor

I can see that overriding versions is a valid method of combining dependencies, but it seems less motivated to me. Can you explain a little bit more about why you want to override version numbers in a way that's incompatible with the previous spec?

I do agree that combining all dependencies w/o overrides seems more reasonable in general, which is also what made me a bit surprised in the first place and motivated me to ask #222. Then (perhaps my bad), I though I could leverage the current "by design" feature to dynamically create and use an additional environment-overrides.yml in a Continuous Integration pipeline, where we want to assess different version of certain dependencies besides those specified in nominal environment YAML files (and already locked): In practice, we re-run conda-lock on-the-fly with an additional environment-overrides.yml, specified last.

I also understand this is a pretty specific use-case, for which we can also foresee an alternative approach at our end if an opt-in possibility is not viable / not worth maintaining.

@knedlsepp
Copy link
Contributor

knedlsepp commented Nov 9, 2023

Found this issue while trying to split the constraints from my conda-env.yml into a separate file and was suprised that conda-lock silently ignored the version constraints for some packages. Apparently this comes from the fact that in

unique_deps[key] = dep
we just overwrite dependencies we already have in the list even if that means overwriting the version field with an empty string.

@maresb Any way we could move this forward?

@maresb
Copy link
Contributor

maresb commented Nov 9, 2023

@knedlsepp, I'd love to get some help with this. See also #390 and #410.

Some good news is that the codebase has improved in significant ways since the previous discussion. Especially relevant is that platforms are now computed independently, and that was a big blocker before.

I think this is fundamentally doable now. Would you be up for starting a new PR?

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.

4 participants