conda / conda-lock

Lightweight lockfile for conda environments
https://conda.github.io/conda-lock/
Other
488 stars 103 forks source link

Support pypi repository on the local disk (`file://` urls) #538

Open FelixSchwarz opened 11 months ago

FelixSchwarz commented 11 months ago

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.

netlify[bot] commented 11 months ago

Deploy Preview for conda-lock ready!

Name Link
Latest commit 04109c00f4a5d6aa205bc840153c7e4592ee749f
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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

Still seeing the same failure after rerunning. :(

FelixSchwarz commented 5 months ago

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 commented 5 months ago

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.