search

conda / conda-lock

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

Proposed fix for #579 #588

Closed ianpye closed 4 months ago

ianpye commented 5 months ago

Description

This PR proposes a simple fix to issue #579. Currently, the --pypi_to_conda_lookup_file flag does not work and produces an error whenever it is used, without checking the link first.

The removal of the following lines allows the flag to work as intended:

del self.pypi_lookup
del self.conda_lookup
netlify[bot] commented 5 months ago

Deploy Preview for conda-lock ready!

Name Link
Latest commit c1e4d2fd5e238389e851f1b1bec468d91ee4ef59
Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/65c808a9deede200085677b4
Deploy Preview https://deploy-preview-588--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.

maresb commented 5 months ago

Thanks a lot @ianpye for taking the initiative on this!

I believe the proper fix is something more like:

        if self._mapping_url != value:
            self._mapping_url = value
            # Invalidate cache
            try:
                del self.pypi_lookup
            except AttributeError:
                pass
            try:
                del self.conda_lookup
            except AttributeError:
                pass

This feature desperately needs a test to prevent a similar regression in the future.

ianpye commented 5 months ago

Thanks @maresb for your input, I have changed my code to reflect those suggestions that you made.

maresb commented 5 months ago

Thanks @ianpye!

I made a quick attempt to write a test, but I didn't manage to get it to work properly yet. Do you have any ideas for a brief test that ensures we are using the specified mapping? I was thinking we could create a mapping URL that swaps some dependency, and then the test checks inside the generated lockfile for that substituted dependency.

maresb commented 5 months ago

Maybe I'm going the wrong way in my gist. I was thinking about constructing an example for pyproject.toml, but you're using only environment.yml, which is probably simpler.

maresb commented 5 months ago

The current test failure on macos is erroneous. See #591.

ianpye commented 5 months ago

@maresb What do you think about having 2 simple tests, one that uses the flag to point to the new mapping url that you provided and one that swaps the link to an invalid link. The 2nd test would check for failure and show that there is an attempt to connect to the link provided.