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

Support multi-file version constraints #546

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

Conversation

knedlsepp
Copy link
Contributor

@knedlsepp knedlsepp commented Nov 10, 2023

Description

This adds support for multi-file version constraints.

Prior work: #300

Copy link

netlify bot commented Nov 10, 2023

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit 8379271
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/656211a5b4a7740008f848c2
😎 Deploy Preview https://deploy-preview-546--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 configuration.

@knedlsepp knedlsepp force-pushed the multi-file-version-constraints branch 9 times, most recently from 6467847 to 5484a88 Compare November 10, 2023 12:36
@maresb
Copy link
Contributor

maresb commented Nov 10, 2023

Please don't be shy about cleaning up the existing code if you're so inclined.

@knedlsepp
Copy link
Contributor Author

Please don't be shy about cleaning up the existing code if you're so inclined.

👍 Although the draft is not me being shy, but rather clumsily trying to make it work at all. ;-)

@knedlsepp knedlsepp force-pushed the multi-file-version-constraints branch 7 times, most recently from 7f44116 to aabc8b9 Compare November 10, 2023 17:20
@knedlsepp
Copy link
Contributor Author

Status update: Versions seem to work. I'm having issues however when a build is specified.

Real world example:

This would work

name: environment
dependencies:
  - openblas=*
  - openblas=0.3.20

(Also when split across files)

The following doesn't work:

name: environment
dependencies:
  - openblas=*=openmp*
  - openblas=0.3.20=openmp_h53a8fd6_1

@maresb
Copy link
Contributor

maresb commented Nov 10, 2023

Very nice. Any idea why the build number is problematic?

@knedlsepp knedlsepp force-pushed the multi-file-version-constraints branch 2 times, most recently from aac263b to 06a2cff Compare November 12, 2023 17:15
@knedlsepp
Copy link
Contributor Author

knedlsepp commented Nov 12, 2023

Very nice. Any idea why the build number is problematic?

Apparently in a conda MatchSpec it is possible to have versions consisting of multiple "constraints", but it is currently not possible to specify build consisting of multiple "constraints". (I found this: conda/conda#11612 )

$ conda search "_openmp_mutex=*=*_llvm,2_kmp_llvm"
InvalidVersionSpec: Invalid version '*=*_llvm': invalid character(s)

$ conda search "_openmp_mutex=*=2_kmp_llvm,*_llvm"
InvalidVersionSpec: Invalid version '*=2_kmp_llvm': invalid character(s)

$ conda search _openmp_mutex[build=*_llvm,2_kmp_llvm] # Part after "," is ignored
Loading channels: - 
done
# Name                       Version           Build  Channel             
_openmp_mutex                    4.5          0_llvm  conda-forge         
_openmp_mutex                    4.5          1_llvm  conda-forge         
_openmp_mutex                    4.5      2_kmp_llvm  conda-forge         

$ conda search _openmp_mutex[build=2_kmp_llvm,*_llvm] # Part after "," is ignored
Loading channels: done
# Name                       Version           Build  Channel             
_openmp_mutex                    4.5      2_kmp_llvm  conda-forge         


$ conda search _openmp_mutex[build=*_llvm,build=2_kmp_llvm] # Later bracket argument is used
Loading channels: done
# Name                       Version           Build  Channel             
_openmp_mutex                    4.5      2_kmp_llvm  conda-forge         


$ conda search _openmp_mutex[build=2_kmp_llvm,build=*_llvm]  # Later bracket argument is used
Loading channels: - 
done
# Name                       Version           Build  Channel             
_openmp_mutex                    4.5          0_llvm  conda-forge         
_openmp_mutex                    4.5          1_llvm  conda-forge         
_openmp_mutex                    4.5      2_kmp_llvm  conda-forge    

$ conda search _openmp_mutex=*=2_kmp_llvm[build=*_llvm] # bracket argument overwrites earlier argument
Loading channels: done
# Name                       Version           Build  Channel             
_openmp_mutex                    4.5          0_llvm  conda-forge         
_openmp_mutex                    4.5          1_llvm  conda-forge         
_openmp_mutex                    4.5      2_kmp_llvm  conda-forge         

The commit I pushed now deals with that by (ab-?)using fnmatch.fnmatchcase to merge the two build strings ("2_kmp_llvm", "*_llvm") into "2_kmp_llvm".

It's working nicely for my real world usage to work around the issue in #386.
For that I render a conda-env-pinned-tmp.yml from an existing lockfile, then I add my new package to conda-env.yml then I run conda-lock using both these files.

conda-lock render --kind env --filename-template conda-env-pinned-tmp
sed -i 's/ --hash/ # --hash/g' conda-env-pinned-tmp.yml # packaging.requirements.InvalidRequirement: Expected end or semicolon (after version specifier)
conda-lock lock --conda `which conda` --file ./conda-env-pinned-tmp.yml --file ./conda-env.yml --lockfile conda-lock.yml --platform linux-64

So now I guess we need some more tests, but I'm optimistic that the current solution is a nice improvement. :)

@knedlsepp knedlsepp force-pushed the multi-file-version-constraints branch 2 times, most recently from 5593f32 to d632ad9 Compare November 14, 2023 20:08
Until conda/conda#11612 is resolved we do a
simplified version of combining build strings.

This is a change in behavior as previously package version constraings
would simply be overwritten instead of combined.
@knedlsepp knedlsepp force-pushed the multi-file-version-constraints branch from d632ad9 to 2dc54e9 Compare November 14, 2023 20:12
@knedlsepp knedlsepp changed the title Attempt supporting multi-file version constraints Support multi-file version constraints Nov 14, 2023
@knedlsepp knedlsepp marked this pull request as ready for review November 14, 2023 20:12
@knedlsepp knedlsepp requested a review from a team as a code owner November 14, 2023 20:12
@knedlsepp
Copy link
Contributor Author

@maresb I think this is now good enough for a first review. The tests aimed at the build part of the matchspec are mostly covering my personal use-cases of specifying mpi and openmp build variants. I did not yet test if this would work for the kind of "overlapping globs" mentioned in conda/conda#11612.

@maresb
Copy link
Contributor

maresb commented Nov 20, 2023

Thanks @knedlsepp! I will try to review this soon. Once I get to it, I hope to release it as v2.6.0.

Copy link
Contributor

@maresb maresb left a comment

Choose a reason for hiding this comment

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

Thanks @knedlsepp! I have a bunch of minor feedback.

This looks like it will be tricky to merge due to breaking changes. I wonder if we could coordinate with @srilman to have this alongside categories.

conda_lock/models/lock_spec.py Outdated Show resolved Hide resolved
conda_lock/models/lock_spec.py Outdated Show resolved Hide resolved
Comment on lines 91 to 93
build = self._merge_matchspecs(
self.build, other.build, combine_constraints=False
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I spent pretty long in confusion here. Rather than use combine_constraints I think it would be better to have a separate functions here, like _merge_version and _merge_build. I have far less qualms about wildcard tricks for build numbers.

Also I think you can get rid of the surrounding try/except block: just raise the exception directly in _merge_build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I think you can get rid of the surrounding try/except block: just raise the exception directly in _merge_build.

The surrounding try/except block adds the package names to the exception, which is convenient for the user to figure out which package has conflicting builds. Otherwise the error message would only contain the build ids

conda_lock/src_parser/aggregation.py Outdated Show resolved Hide resolved
conda_lock/models/lock_spec.py Outdated Show resolved Hide resolved
We need to keep the "# type: ignore" because otherwise we get the
following error in mypy:

- conda_lock/src_parser/aggregation.py:42: error: Argument 1 to "merge"
  of "VersionedDependency" has incompatible type "VersionedDependency |
  URLDependency | VCSDependency | None"; expected "VersionedDependency |
  None"  [arg-type]
- conda_lock/src_parser/aggregation.py:42: error: Argument 1 to "merge"
  of "URLDependency" has incompatible type "VersionedDependency |
  URLDependency | VCSDependency | None"; expected "URLDependency | None"
  [arg-type]
- conda_lock/src_parser/aggregation.py:42: error: Argument 1 to "merge"
  of "VCSDependency" has incompatible type "VersionedDependency |
  URLDependency | VCSDependency | None"; expected "VCSDependency | None"
  [arg-type]
@knedlsepp
Copy link
Contributor Author

Thanks for the review. I did apply your suggestions. Still need to look closer into the category -> categories change.

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.

2 participants