conda / conda-lock

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

Update the `seperator_munge_get` to provide flexibility for dependency lookup #398

Closed leeleavitt closed 1 year ago

leeleavitt commented 1 year ago

The fix in this PR adresses two example environment yamls,

In this example bullet-python a dependency of pytest-wdl breaks seperator_munge_get

name: python
platforms:
  - linux-64
  - osx-64
channels:
  - bioconda
  - conda-forge
dependencies:
  - python=3.9.12
  - pip
  - miniwdl=1.2.3
  - pip:
    - pytest-wdl

In this example python-tzdata breaks seperator_munge_get,

name: python
platforms:
  - linux-64
  - osx-64
channels:
  - bioconda
  - conda-forge
dependencies:
  - python=3.9.12
  - pybedtools
  - pip
  - pip:
    - pandas==1.3.5
netlify[bot] commented 1 year ago

Deploy Preview for conda-lock ready!

Name Link
Latest commit 8c79278b38468168c5bd852a989fac859c47890e
Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/643715c0a0c34f0008418f76
Deploy Preview https://deploy-preview-398--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 commented 1 year ago

Thank you very much for this @leeleavitt!!!

I have been meaning to dig into this function to try to understand exactly what it's supposed to accomplish. There are some very helpful comments in #290 about what the surrounding code is doing.

I would like to merge #290 very soon. Would you be able to test your update with that branch?

Also, since you have spent some time on separator_munge_get, do you understand exactly what it is doing? If so, would you be able to explain it? In particular, I am wondering why for pip dependencies we aren't using canonicalize_name, and I am also wondering what exactly are the "inconsistencies" within Conda.

Cakell commented 1 year ago

Hi @maresb .

Regarding the logic behind separator_munge_get, see #139 and in particular this commit.

TL;DR:

This code is meant to "Add some slightly more robust fallback handling to find packages that have different separators when using a mixed poetry+conda solve".

Hopes this helps.

PS. I also came across this separator_munge_get bug, and would love to see a release which fixes this soon : )

maresb commented 1 year ago

Thanks @Cakell, does this PR solve the bug for your case?

olivier-lacroix commented 1 year ago

Thanks @Cakell, does this PR solve the bug for your case?

It does for me! encountered the issue with python-tzdata

leeleavitt commented 1 year ago

I will see if i can refactor the code to get around the flake8 complexity failure. Is there a specific version of flake8 or black to use? I couldn't see a version pinned in the repo. Should i just go with the latest versions?

Also @maresb in regards to testing #290 should I merge that branch into mine and then test it?

maresb commented 1 year ago

Thanks @leeleavitt!

To get around the complexity failure, I think you could simply do

try:
    ...
except KeyError:
   pass

and this should allow you to dedent the rest, and I expect this to decrease your complexity score.

should I merge that branch into mine and then test it?

Exactly. Thanks so much for your work on this, I really appreciate it!

leeleavitt commented 1 year ago

Thanks @leeleavitt!

To get around the complexity failure, I think you could simply do

try:
    ...
except KeyError:
   pass

and this should allow you to dedent the rest, and I expect this to decrease your complexity score.

should I merge that branch into mine and then test it?

Exactly. Thanks so much for your work on this, I really appreciate it!

Alright i spent some time testing this yesterday and #290 fixes the issue without the need for my fix. I'm going to write up my findings on that PR.

lesteve commented 1 year ago

Actually I did not see this one before opening #401 but it looks like both are related. Note #401 is NOT fixed by #290.

Removing python- does not seem like super robust. I think there should be a way somewhere to say something like:

mariusvniekerk commented 1 year ago

Thanks a lot for your PR.

As @lesteve pointed out this is not a generally robust solution and special cases should be dealt with by making pull requests to the upstream location wheree the mapping is defined over here https://github.com/regro/cf-scripts/blob/master/conda_forge_tick/pypi_name_mapping_static.yaml

As such I'm closing this PR.