Fix #253: package confusion between conda and pip #290

romain-intel · 2022-12-01T22:45:55Z

Before this patch, the following would fail:

channels:
  - conda-forge
dependencies:
  - pip
  - dask
  - pip:
    - dask

The core issue seems to be that two Conda packages (dask and dask-core) both map to the dask pypi package. This was causing issues later on when assigning categories. This patch properly deals with multiple Conda packages per pip package and addresses the naming issue.

I have verified that the above package specification works as well as the others listed in #253.

Before this patch, the following would fail: ``` channels: - conda-forge dependencies: - pip - dask - pip: - dask ``` The core issue seems to be that two Conda packages (`dask` and `dask-core`) both map to the `dask` pypi package. This was causing issues later on when assigning categories. This patch properly deals with multiple Conda packages per pip package and addresses the naming issue. I have verified that the above package specification works as well as the others listed in conda#253.

netlify · 2022-12-01T22:46:02Z

✅ Deploy Preview for conda-lock ready!

Name	Link
🔨 Latest commit	`ffcb2d5`
🔍 Latest deploy log	https://app.netlify.com/sites/conda-lock/deploys/63892ea559623f0008997a35
😎 Deploy Preview	https://deploy-preview-290--conda-lock.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

romain-intel · 2022-12-09T16:33:05Z

I think there is some test flakiness given that it passes on other Python versions/platforms. Let me know if it is something else though.

…confusion

netlify · 2023-02-09T22:35:43Z

✅ Deploy Preview for conda-lock ready!

Name	Link
🔨 Latest commit	`452ff6b`
🔍 Latest deploy log	https://app.netlify.com/sites/conda-lock/deploys/6464fba34ca0800007ae8c40
😎 Deploy Preview	https://deploy-preview-290--conda-lock.netlify.app
📱 Preview on mobile	Toggle 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 · 2023-02-25T17:33:20Z

pre-commit.ci autofix

romain-intel · 2023-02-25T21:43:03Z

Thanks for taking a look. I'll fix the pre-commit thing (I just need to extract a function out -- I had it in my first version and then undid it because I had forgotten why -- it was to remove the too complex function warning). I'll check the type issues too. May have missed something with the merge.

maresb · 2023-02-25T21:46:54Z

Awesome, thanks!!! I really want to try and get this in soon, but I still need to understand it first. Be warned there could be further merge conflicts.

maresb · 2023-02-25T22:54:56Z

Any thoughts on #322?

maresb · 2023-03-02T21:22:05Z

Closes #380

romain-intel · 2023-03-03T21:16:59Z

Sorry, I have been traveling -- I haven't forgotten. I'll try to get to it tonight or over the weekend.

…confusion

romain-intel · 2023-03-10T21:24:46Z

I will see if I can add a test as well.

romain-intel · 2023-03-10T22:54:39Z

Any thoughts on #322?

I must say that I am not fully well versed in the mapping rules. My gut feeling though is that @PanayotisManganaris is probably right in saying that there are probably cases where the mapping done will go wrong and it may be nice to offer users a "way out". There is currently no option to change to a different lookup table and/or to add to it. I could see it becoming quite possibly complicated though:

different lookup tables for different channels
ability for users to specify their own yaml lookup (that should be fairly straightforward to enable and may cover a large majority of these cases
ability for users to specify overrides in the yaml file to resolve (that may also cover a good variety of use cases).

romain-intel · 2023-03-10T22:55:49Z

OK, I added a simple test (nothing fancy) but hopefully this should be good to go. Apologies again for the delay -- it's been a very hectic few weeks but let me know if you need anything else. Would love to get this merged too (and released -- it's been a while since the last release :) ).

maresb · 2023-03-10T23:25:39Z

Thank you so much for this!!!

I really want to get this merged too, and I agree that we're due for a big release soon.

I am also getting somewhat lost with how the mapping is supposed to work. Now that you have your head wrapped around this part of the logic, would you have any time to add a few comments? I find the code in this vicinity really difficult to read, not just this PR. Alternatively, maybe I could try an figure it out myself, and you could review what I write to see if it's accurate.

Some of the things I'm getting confused about: what is the planned mapping? In particular, what exactly are the keys, and why are the values lists of LockedDependencys? What is the general iteration strategy with extract_planned_items? And it what sense are they "extracted"? (Maybe we should be using a different word here?)

romain-intel · 2023-03-11T21:10:41Z

Let me see what I can add. I can't say I have a brilliant understanding of the code but can try to clarify some of the portions I understood. Hopefully with both of us we can come up with something sensible :).

…confusion

maresb · 2023-04-03T10:14:24Z

I'm extremely time-constrained at the moment, and I still understand neither the old nor the new logic.

However, there seem to be a lot of people running into #253, and this patch seems to be extremely successful in resolving it. This is also not a radical change. @mariusvniekerk, I feel like this should be safe to merge, and then on the next iteration we can work on documentation and simplification.

Complementary to this effort, I do understand the upstream PyPI mapping quite a bit better, and I recently made a bunch of improvements. So in case the complications in the current code are to make up for previous insufficiencies in the PyPI mapping, this may allow for some significant simplifications.

romain-intel · 2023-04-05T23:33:40Z

hello -- apologies. I said I would add some comments and then the end of the quarter came. Now that it is done, your message came at the perfect time so I made some time to put at least some comments. I can't say that I fully understand why it is doing this category assignation and what the role of it is but hopefully it clarifies how it is doing it. Let me know if this helps and if you need something else.

leeleavitt · 2023-04-21T15:48:39Z

I composed 3 manual tests,

pyproject.toml

[build-system]
requires = ["poetry-core>=1.0.0"]
build-backend = "poetry.core.masonry.api"

[tool.poetry]
name = "tester"
version = "0.1.0"
description = "tester project"
requires-python = ">=3.9"

# Conda lock environment specs
[tool.conda-lock]
channels = [
    "bioconda",
    "conda-forge"
]
platforms = [
    "linux-64",
    "osx-64"
]

[tool.poetry.dependencies]
python = "3.9.12"
pip = "*"
pybedtools = "0.9.0"
miniwdl = "1.2.3"
pytest-wdl = {version = "*", source = "pypi"}

zenvironment-pytest-wdl.yaml

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

zenvironment-python-tzdata.yaml

name: python
platforms:
  - linux-64
  - osx-64
channels:
  - bioconda
  - conda-forge
dependencies:
  - python=3.9.12
  - pybedtools
  - pip
  - pip:
    - pandas==1.3.5

I first tested merging your branch into mine for #398, i did not get the errors. So then i removed my fix and tested my examples on your branch and i did not get the errors.

I went one step further and removed _separator_munge_get to simply,

def _seperator_munge_get(
   d: Mapping[str, Union[List[LockedDependency], LockedDependency]], key: str
) -> Union[List[LockedDependency], LockedDependency]:
   # since separators are not consistent across managers (or even within) we need to do some double attempts here
   # try:
   return d[key]

And once again i did not get any error. Overall i believe this fixes my issues and i would love to see this get merged into main.

maresb · 2023-04-21T16:20:27Z

I went one step further and removed _separator_munge_get to simply...

Awesome, this is really great info @leeleavitt! It would be great to be able to remove it since it's so confusing.

I also really appreciate the effort to design those tests. Did you implement them in PyTest? If so, would you be willing to put them in a PR? That would certainly help me with gathering the courage to merge this.

leeleavitt · 2023-04-21T19:34:21Z

I went one step further and removed _separator_munge_get to simply...

Awesome, this is really great info @leeleavitt! It would be great to be able to remove it since it's so confusing.

I also really appreciate the effort to design those tests. Did you implement them in PyTest? If so, would you be willing to put them in a PR? That would certainly help me with gathering the courage to merge this.

I was looking into the testing organization, but i could not see how to add these as tests. Do you have any pointers for where and how i could add these as tests? Since these would require network interaction i'm assuming I would need to create some cassettes?

romain-intel · 2023-04-25T20:12:59Z

Re _separator_munge_get: I did not change the function in my PR because I wasn't sure if there was a reason for it (chesterton's fence). I believe that it is there to make names that have _ or - the same as they are sometimes used interchangeably.

In terms of tests, I think you can add the .yml files just like I added the one test file for the one I ran. Is that what you are asking?

lesteve · 2023-04-28T05:41:45Z

I believe that it is there to make names that have _ or - the same as they are sometimes used interchangeably.

In my experience pip does not care about - or _ (or . by the way) e.g. pip install scikit_learn or pip install scikit-learn (see normalization PyPA doc also). I believe this kind of caveats should be supported in conda-lock as well so _separator_munge_get should stay ...

It would be really great to merge this, this would get rid of a lot of issues with mixed conda/pip dependencies.

mariusvniekerk · 2023-05-17T13:51:13Z

@maresb Added these tests in. Will merge if they pass

maresb · 2023-05-17T13:52:36Z

Excellent, thanks a lot @mariusvniekerk for moving this forward!

mariusvniekerk · 2023-05-17T13:56:02Z

Probably worthwhile consolidating the various simple "solvability tests" at some point, but out of scope here

maresb · 2023-05-17T15:20:58Z

@mariusvniekerk I'd really like to get your thoughts on name canonicalization. For PyPI there is a canonicalize_name function, so I think if we canonicalize PyPI names everywhere then there should no longer be a problem with those.

Then the only question is Conda, which to the best of my knowledge is trickier. I patched a bunch of the upstream Grayskull mapping stuff, and I'm wondering if there is any know example where seperator_munge_get actually does something?

romain-intel · 2023-05-17T15:44:28Z

Whoohoo! Thanks for moving this forward! If the tests don't pass, I'll take a look and see if there is something I can do.

mariusvniekerk · 2023-05-17T16:07:59Z

conda_lock/pypi_solver.py

@@ -228,6 +228,9 @@ def solve_pypi(
    for locked_dep in conda_locked.values():
        if locked_dep.name.startswith("__"):
            continue
+        # ignore packages that don't depend on Python
+        if locked_dep.manager != "pip" and "python" not in locked_dep.dependencies:


Pretty certain this change alone solves basically everything

That's intriguing!!! When does it trigger?

What's an example of a non-Python pkg which causes trouble?

tzdata is the classic one, so adding in the contraint causes pip to never attempt to assume it has tzadata already installed since it doesn't

I'm concerned about the failing tests on Windows...

The failures between the last two runs were disjoint. Maybe we just got unlucky with transient errors. Trying a third time.

mariusvniekerk · 2023-05-18T02:05:51Z

Then the only question is Conda, which to the best of my knowledge is trickier. I patched a bunch of the upstream Grayskull mapping stuff, and I'm wondering if there is any know example where seperator_munge_get actually does something?

Sadly conda packages have no canonical name form (practically many things are lowercase kebab, but it's not a hard rule and some things even use BOTH)

romain-intel · 2023-05-18T16:37:50Z

WHOOHOO!! Thanks for merging this!! Much appreciated! Now I can get off my custom build :)

lesteve · 2023-05-19T13:13:15Z

Great to see this merged, thanks to everyone involved, this will get rid of a lot of issues with mixed conda/pip dependencies!

Merge remote-tracking branch 'upstream/main' into fix/pypi-conda-pkg-…

c960805

…confusion

romain-intel requested a review from a team as a code owner February 9, 2023 22:35

Fix typos

2ea5ba0

maresb force-pushed the fix/pypi-conda-pkg-confusion branch from a84fe26 to 36f0985 Compare February 25, 2023 17:54

maresb mentioned this pull request Mar 2, 2023

Fails to resolve for dependency with a dash (-) in name #380

Closed

2 tasks

maresb linked an issue Mar 2, 2023 that may be closed by this pull request

Fails to resolve for dependency with a dash (-) in name #380

Closed

2 tasks

maresb mentioned this pull request Mar 4, 2023

pyproject.toml dependency with caps not canonicalized #382

Open

2 tasks

romain-intel force-pushed the fix/pypi-conda-pkg-confusion branch from 36f0985 to 2ea5ba0 Compare March 10, 2023 20:33

romain-intel added 2 commits March 10, 2023 12:35

Merge remote-tracking branch 'upstream/main' into fix/pypi-conda-pkg-…

e0b3286

…confusion

Fix pre-commit issues

594e98c

Added test

e6da6a4

Merge remote-tracking branch 'upstream/main' into fix/pypi-conda-pkg-…

156b2b0

…confusion

epassaro mentioned this pull request Mar 30, 2023

conda_lock._vendor.poetry.utils._compat.CalledProcessError: Command returned non-zero exit status 1 #396

Open

2 tasks

Added comments

684f00e

maresb mentioned this pull request Apr 12, 2023

Update the seperator_munge_get to provide flexibility for dependency lookup #398

Closed

lesteve mentioned this pull request Apr 28, 2023

Issue with mixed conda/pip dependencies when a name does not mean the same package in conda and pip #401

Closed

2 tasks

Add tests from conda#290

a82c739

mariusvniekerk added 2 commits May 17, 2023 11:06

Fix test param

3bf3d65

Add mitigation for urllib3 2.0

5a33053

Fixup test

dd39b60

Don't attempt to lock non-python packages in the poetry solver

452ff6b

mariusvniekerk reviewed May 17, 2023

View reviewed changes

maresb approved these changes May 17, 2023

View reviewed changes

mariusvniekerk merged commit 4a667bd into conda:main May 18, 2023
11 checks passed

maresb mentioned this pull request May 18, 2023

Solver can fail with mixed pip/conda dependencies #179

Open

cameronraysmith mentioned this pull request May 18, 2023

conda-lock 1.4.1 fails to generate lock file from pyproject.toml with mixed conda-forge/PyPI dependencies pinellolab/pyrovelocity#55

Closed

maresb mentioned this pull request May 20, 2023

Remove optional From Dependency Info in Internal Code #389

Merged

robertodr mentioned this pull request Oct 25, 2023

Cannot generate lock for project depending on Dask #539

Closed

2 tasks

Fix #253: package confusion between conda and pip #290

Fix #253: package confusion between conda and pip #290

Conversation

romain-intel commented Dec 1, 2022

netlify bot commented Dec 1, 2022 • edited

✅ Deploy Preview for conda-lock ready!

romain-intel commented Dec 9, 2022

netlify bot commented Feb 9, 2023 • edited

✅ Deploy Preview for conda-lock ready!

maresb commented Feb 25, 2023

romain-intel commented Feb 25, 2023

maresb commented Feb 25, 2023

maresb commented Feb 25, 2023

maresb commented Mar 2, 2023

romain-intel commented Mar 3, 2023

romain-intel commented Mar 10, 2023

romain-intel commented Mar 10, 2023

romain-intel commented Mar 10, 2023

maresb commented Mar 10, 2023

romain-intel commented Mar 11, 2023

maresb commented Apr 3, 2023

romain-intel commented Apr 5, 2023

leeleavitt commented Apr 21, 2023

pyproject.toml

zenvironment-pytest-wdl.yaml

zenvironment-python-tzdata.yaml

maresb commented Apr 21, 2023

leeleavitt commented Apr 21, 2023

romain-intel commented Apr 25, 2023

lesteve commented Apr 28, 2023 • edited

mariusvniekerk commented May 17, 2023

maresb commented May 17, 2023

mariusvniekerk commented May 17, 2023 • edited

maresb commented May 17, 2023

romain-intel commented May 17, 2023

mariusvniekerk May 17, 2023

Choose a reason for hiding this comment

maresb May 17, 2023

Choose a reason for hiding this comment

maresb May 17, 2023

Choose a reason for hiding this comment

mariusvniekerk May 17, 2023 • edited

Choose a reason for hiding this comment

maresb May 17, 2023

Choose a reason for hiding this comment

maresb May 17, 2023

Choose a reason for hiding this comment

mariusvniekerk commented May 18, 2023

romain-intel commented May 18, 2023

lesteve commented May 19, 2023

netlify bot commented Dec 1, 2022 •

edited

netlify bot commented Feb 9, 2023 •

edited

lesteve commented Apr 28, 2023 •

edited

mariusvniekerk commented May 17, 2023 •

edited

mariusvniekerk May 17, 2023 •

edited