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

Refactor LockSpecification as a Dictionary from Platforms to List of Deps #383

Merged
merged 10 commits into from
Mar 11, 2023

Conversation

srilman
Copy link
Contributor

@srilman srilman commented Mar 5, 2023

Description

As discussed in #374, #278, and previously implemented in #316, this PR refactors the LockSpecification class to store all dependencies as a dictionary from platforms to a list of dependencies required for the platform.

After this PR, it is no longer necessary to have the Selector class, so it is also removed in this PR.

This PR still maintains source parsing code to return LockSpecification classes. This is not an ideal solution, since it requires parsing the source file multiple times. In the future, I will implement a PR to have a custom SourceFile class, like implemented in #316.

@srilman srilman requested a review from a team as a code owner March 5, 2023 00:05
@netlify
Copy link

netlify bot commented Mar 5, 2023

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit 3ce07dc
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/64051de18a77c40008699c44
😎 Deploy Preview https://deploy-preview-383--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.

@maresb
Copy link
Contributor

maresb commented Mar 5, 2023

This is magnificent!!! I love that we were able to delete so much code. It's nice to see the CI green. And now the code logic agrees with my mental model. All this makes very happy. 😄

During my review I cleaned up some of the preexisting code a bit further: srilman#1. If you agree, let's wrap that into this PR by merging on your fork.

I've reviewed this thoroughly, and from my side this is good to go.

Since this is such a substantial structural change, I think we should get approval from @mariusvniekerk.

@srilman
Copy link
Contributor Author

srilman commented Mar 5, 2023

@maresb I'm happy with the additional code cleanup in my local branch. I'm going to merge your branch into this one.

Filtering categories is opt-in, thus the default of True is wrong.
There is only one active usage of `make_lock_files` which sets
`filter_categories` explicitly, so changing this value shouldn't
break anything.
These lists are not optional, so `or []` doesn't seems to accomplish
anything.
This is obsolete since we no longer use aggregate_lock_specs to
aggregate specs from separate platforms.
We shouldn't compute `platforms` again in a different way. Instead
we pass the value we already computed, and then we perform a
consistency check whenever `len(lock_specs) > 0`.
@maresb
Copy link
Contributor

maresb commented Mar 6, 2023

For orientation as to how this fits into the big picture:

What conda-lock does in a nutshell:

  • Each source file (environment.yml, pyproject.toml, etc.) is preprocessed in order to compute the target platforms (linux-64, osx-64, etc.).
  • With the target platforms in hand, each source file is processed in order to extract the dependencies required for each platform. The result is a list of LockSpecifications, one for each source file.
  • The LockSpecifications from the individual source files are aggregated into a single LockSpecification.
  • The aggregated LockSpecification is solved for each platform, resulting in a big list of LockedDependency objects.
  • These LockedDependency objects are packed with various metadata into a Lockfile object.
  • The Lockfile object is converted into the desired output format.

This PR reworks the internally-used LockSpecification data structure so that its implementation is more in line with how it's actually used.

@srilman
Copy link
Contributor Author

srilman commented Mar 6, 2023

To clarify further, this will not change how we parse input files or the resulting output lockfile in any way. This will only change an internal data structure so that it is easier to implement future changes. For example, I was working on a future PR to support encoding multiple categories in the lockfile, but kept running into bugs because of how we grouped all platforms together in the LockSpecification.

@maresb maresb merged commit aaf311a into conda:main Mar 11, 2023
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.

None yet

2 participants