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 pypi repository on the local disk (file:// urls) #538

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

Conversation

FelixSchwarz
Copy link

@FelixSchwarz FelixSchwarz commented Oct 24, 2023

This is a quick PR to support file:// urls which resolves #536 .

Since #529 there is support for additional pip repositories. With this PR also local repositories are supported, specified by a file:// url like this:

channels:
  - conda-forge
pip-repositories:
  - file:///some/path/to/local/repo/
dependencies:
  - python=3.11
  - requests=2.26
  - pip:
    - fake-private-package==1.0.0

Why is this needed?

We require some private wheels for our repo but there is no private registry in place. The easiest thing for us seems to be to put the wheels in git lfs which is available for every developer. That way we do not need to operate a private pypi service and we circumvent all the potential problems with regards to authentication/user accounts.

Running conda-lock with a file:// url triggers an exception by requests:
requests.exceptions.InvalidSchema: No connection adapters were found for 'file:///some/path/to//'.

As a follow-up I will submit a patch to the requests-file project but only if conda-lock actually needs it.

@FelixSchwarz FelixSchwarz requested a review from a team as a code owner October 24, 2023 16:19
@netlify
Copy link

netlify bot commented Oct 24, 2023

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit 04109c0
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/654e830a99f13e0008d4ba0c
😎 Deploy Preview https://deploy-preview-538--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.

@FelixSchwarz
Copy link
Author

FelixSchwarz commented Oct 24, 2023

The code is not really nice, I especially dislike that I had to basically copy the code for Factory.create_legacy_repository and I thought about moving it to the interfaces package just to have it out of the way. Also it actually increases your dependency on poetry with all this get_client_cert() things but that is how I could implement the functionality...

At least the required changes to requests-file are clearly visible as I created two commits.

@maresb
Copy link
Contributor

maresb commented Oct 24, 2023

Thanks a lot for this!

The code is not really nice

At the absolute level maybe not, but given the approach this seems pretty good to me at first glance.

Regarding the changes to requests-file, this should use vendoring an with a patch configured here, but that's quite a chunk of specialty tooling, so I could take care of that for you.

It may take me some days to give this a proper review. I'd also request a few tests, but those could wait until after the first review in case you think we might come up with a better approach.

@FelixSchwarz
Copy link
Author

I thought a bit about this but in the end I can not think of a better approach unless we write a Repository implementation from scratch.

Maybe you can indicate some tests you would like to see? I expect something like create some static pypi html in a temp directory + a private dummy package and check that conda-lock can install it? I guess the test_pip_repositories.py in general would be a good start to add tests? Any other specific test cases you would like to see?

@maresb
Copy link
Contributor

maresb commented Oct 24, 2023

That all sounds good.

I don't expect this to be a common use-case, but for robustness it'd be nice to have a test which exercises both http and file repos at the same time.

@FelixSchwarz
Copy link
Author

@maresb I added a simple test case so I hope you can start a review, maybe we can get this in soon-ish? I guess there will be quite a few things that need to be polished, but let's hope nothing too serious.

I created a new test file because the html has subtle differences but I dislike that there is a bit of duplication with the original pypi repo test. If you have some ideas for improvments, I am all ears.
Another aspect is injecting the actual file url into the environment.yaml. I could use a static file url by using something like pyfakefs but then decided it to keep the number of dependencies lower in exchange for this .replace("PYPI_FILE_URL", ...).

@maresb
Copy link
Contributor

maresb commented Nov 10, 2023

Moin @FelixSchwarz! Sorry to take so long to come back to this.

I had a look and it seems very good. I did a pass of making some changes to make things a bit more logical in my mind, but don't be shy about reverting anything you don't like, and let me know if there's something you think I misunderstood.

When I run the test locally it freezes without timing out while trying to access https://pypi.org/pypi/six/json. I'm not sure what that's about, but it's a bit unsettling. Hopefully works in CI.

@maresb
Copy link
Contributor

maresb commented Nov 10, 2023

Oh, I forgot to mention: could you please try opening a PR in requests-file for this? That would be a lot better than the current vendoring solution.

@FelixSchwarz
Copy link
Author

FelixSchwarz commented Nov 11, 2023

Thank you for improving my PR. I'll create a PR for requests-file, just give me a few days.

In CI there was a test failure related to mamba on Windows. I saw that micromamba tests are skipped there but I guess mamba should work? If so, I need to dust of a Windows VM and try running the tests there. Do I need some special setup on Windows to run the tests (e.g. do I really need to install docker?)

@maresb
Copy link
Contributor

maresb commented Nov 11, 2023

Ya, that failure seems quite odd. I'm rerunning the test in hopes that we get lucky.

E       conda_lock._vendor.poetry.mixology.failure.SolveFailure: Because -dummy-package- depends on six (*) which doesn't exist, version solving failed.

Windows is always a pain to debug. You could try debugging through GitHub, but that's slow and non-interactive. Marius disabled a bunch of the tests because they were slow and flaky, but we should probably reenable them.

You shouldn't need to run Docker, and really the most important test to get running is yours. So if there's a windows-specific problem with your test and you fix it in a VM and then everything passes in CI I think that's good enough.

Ah, I also wanted to ask you how this handles special characters like spaces and %. Is there any URL-decoding? (I can't think of any situations where the input needs to be encoded, so perhaps it's best to leave out decoding?) It may be nice to add some tightly-scoped unit test (without solving or with trivial solving) to verify that a dummy package can be located if it lives in a path with a difficult name. Since the user has control over where to place the packages, I don't think we should worry about crazy cases like a directory literally named % \/ß#, but it would be nice to check spaces and umlauts.

Thanks for your work on this, I think it's a really valuable feature!

@maresb
Copy link
Contributor

maresb commented Nov 11, 2023

Still seeing the same failure after rerunning. :(

@FelixSchwarz
Copy link
Author

I just checked again and the problem is this: requests_file uses urlparse() on the URL and checks that .netloc is either None or "localhost". On Windows url_parts = urlparse('file://C:/Users/FOO/AppData/Local/Temp/pytest-of-FOO/pytest-7/test_it_installs_packages_from0/repo/six/') the .netloc attribute is C: so requests_file does not work on that URL.

I can adapt the PR but I'm not sure what to do here - also I don't want to spend too much time fixing something just for Windows (which I do not use).

@maresb
Copy link
Contributor

maresb commented Apr 17, 2024

Wow, thanks so much @FelixSchwarz for the deep dive on Windows. I share your apathy for that platform, but we have users depending on it.

I just checked and requests-file just had a release 3 months ago, so if you think you can fix it, let's make the changes here and then submit upstream PRs if that works well for you.

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.

custom pypi repository on the local disk (file:// urls)
2 participants