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

Configure private pip repositories in the environment file #529

Merged

Conversation

jacksmith15
Copy link
Contributor

Description

Note

This is a rebase of #481 with a tidier commit stack.

This PR adds support for specifying private pip repositories in the environment.yml file, similar to how channels are specified.

Similarly to channels, environment variables may be specified, and these will remain as references in the lockfile.

channels:
  - conda-forge
pip-repositories:
  - http://$PIP_USER:$PIP_PASSWORD@private-pypi.org/api/pypi/simple
dependencies:
  - python=3.11
  - requests=2.26
  - pip:
    - fake-private-package==1.0.0

This aims to solve #460.

@jacksmith15 jacksmith15 requested a review from a team as a code owner October 14, 2023 11:35
@netlify
Copy link

netlify bot commented Oct 14, 2023

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit 0cbbd79
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/652e7d15bca19b0009909933
😎 Deploy Preview https://deploy-preview-529--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.

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.

This is really amazing. This looks like it was a huge amount of work! Found a tiny bit of time to look at this. I have a few questions about suffix_union...

conda_lock/models/lock_spec.py Show resolved Hide resolved
conda_lock/models/pip_repository.py Outdated Show resolved Hide resolved
conda_lock/src_parser/aggregation.py Outdated Show resolved Hide resolved
tests/test_pip_repositories.py Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@maresb
Copy link
Contributor

maresb commented Oct 15, 2023

Sorry for not getting to this sooner. The data structure for what I review is more of a stack than a queue, and the previous PR had unfortunate timing with my availability. Thanks so much for your patience and the rebase!!!

Co-authored-by: Ben Mares <services-git-throwaway1@tensorial.com>
@jacksmith15
Copy link
Contributor Author

@maresb I'm not sure why the single test is failing on windows, It doesn't appear relevant to the changes I've made. Possibly related to this being a fork PR?

@maresb
Copy link
Contributor

maresb commented Oct 17, 2023

I think the Windows runner is getting overloaded. It's not specific to this PR. Don't worry about it, and I can force the merge if needed.

@maresb
Copy link
Contributor

maresb commented Oct 17, 2023

I added a test for the pip repo aggregation in jacksmith15#1

@maresb
Copy link
Contributor

maresb commented Oct 17, 2023

I don't think we know for sure that the repository order is actually respected, do we?

How hard would it be to add a test using your nifty framework to check that if we have two repositories a and b, then the first listed is the first one to be queried?

@jacksmith15
Copy link
Contributor Author

I don't think we know for sure that the repository order is actually respected, do we?

I believe this is poetry's behaviour. The find_packages method of Pool checks repositories in the order they were provided to the pool:

https://github.com/conda/conda-lock/blob/main/conda_lock/_vendor/poetry/repositories/pool.py#L169-L170

This method is called by the solver here.

How hard would it be to add a test using your nifty framework to check that if we have two repositories a and b, then the first listed is the first one to be queried?

It would be a chunk of code, and we'd mostly be testing poetry, but happy to add that if you want it confirmed?

@jacksmith15
Copy link
Contributor Author

jacksmith15 commented Oct 17, 2023

Actually that's not entirely true! It looks like Poetry sorts the versions from all available repositories:

https://github.com/conda/conda-lock/blob/1544dc50b3359b3d04b2e75689b3c717b8bcd4e4/conda_lock/_vendor/poetry/puzzle/provider.py#L141C1-L148C1

So it will pick the latest version from among all of them.

If the latest matching version is available in multiple repositories, it will follow the priority order as before, but if a repository further down the priority order has a later version, it will take that one.

I suppose that means we don't need the unifying logic here at all

EDIT: Actually the unifying logic does still make sense in the case where the same versions are available in multiple repositories, as there is still a priority order to respect in this case.

Co-authored-by: Ben Mares <services-git-throwaway1@tensorial.com>
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 @jacksmith15!!!

@maresb
Copy link
Contributor

maresb commented Oct 17, 2023

I'm thinking that ultimately it probably doesn't make sense to merge channels. It would seem more logical to me if each package were solved within the context of only the sources defined in the same specification file. But I don't know that Conda and Poetry are capable of per-package sources.

But rewriting everything from scratch is a bit out-of-scope 😂

@jacksmith15
Copy link
Contributor Author

I'm thinking that ultimately it probably doesn't make sense to merge channels. It would seem more logical to me if each package were solved within the context of only the sources defined in the same specification file. But I don't know that Conda and Poetry are capable of per-package sources.

But rewriting everything from scratch is a bit out-of-scope 😂

Yes unfortunately the poetry solver doesn't support this last time I checked. Its a problem that I haven't seen considered in dependency resolution algorithms.

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