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

Fix case when using multiple platforms but pip contains a single platform #564

Merged
merged 5 commits into from
Dec 1, 2023

Conversation

basnijholt
Copy link
Contributor

@basnijholt basnijholt commented Nov 30, 2023

This came up in basnijholt/unidep#23. This UniDep is a package that unifies Conda and Pip dependency specification and integrates with conda-lock. I think conda-lock folks might like it 😄

About this fix, it prevents:

  File "/Users/bas.nijholt/micromamba/lib/python3.11/site-packages/conda_lock/src_parser/environment_yaml.py", line 58, in _parse_environment_file_for_platform
    for spec in mapping_spec["pip"]:
TypeError: 'NoneType' object is not iterable

Which occurs when locking:

name: example
channels:
  - conda-forge
dependencies:
  - tomli
  - pip:
    - psutil  # [linux64]
platforms:
  - linux-64
  - osx-arm64

Which becomes:

env_yaml_data = {'name': 'test-pip-with-platform-selector', 'channels': ['conda-forge'], 'dependencies': ['tomli', {'pip': None}], 'platforms': ['linux-64', 'osx-arm64']}

For osx-arm64.

This fix skips {"pip": None}.

@basnijholt basnijholt requested a review from a team as a code owner November 30, 2023 01:34
Copy link

netlify bot commented Nov 30, 2023

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit 6ee68f1
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/656a033e8e990c0008d2b5da
😎 Deploy Preview https://deploy-preview-564--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.

…form

Prevents
```
  File "/Users/bas.nijholt/micromamba/lib/python3.11/site-packages/conda_lock/src_parser/environment_yaml.py", line 58, in _parse_environment_file_for_platform
    for spec in mapping_spec["pip"]:
TypeError: 'NoneType' object is not iterable
```
Which occurs when locking:
```
name: example
channels:
  - conda-forge
dependencies:
  - tomli
  - pip:
    - psutil  # [linux64]
platforms:
  - linux-64
  - osx-arm64
```
Which becomes:
```
env_yaml_data = {'name': 'test-pip-with-platform-selector', 'channels': ['conda-forge'], 'dependencies': ['tomli', {'pip': None}], 'platforms': ['linux-64', 'osx-arm64']}
```
For `osx-arm64`.

This fix skips `{"pip": None}`.
@maresb
Copy link
Contributor

maresb commented Nov 30, 2023

Ah, took me a bit to figure out exactly what this is about. Here's my summary:

For the osx-arm64 platform, after filtering

  - pip:
    - psutil  # [linux64]

becomes

  - pip:

then we parse this like

yaml.safe_load("pip:")  # {'pip': None}

so we must deal with the edge case mapping_spec["pip"] = None and not just mapping_spec["pip"] = [].

This looks great! Only thing is that this introduces an orphaned environment.yml in the tests directory. Would you be able to write a test to check that your environment.yml parses correctly and that psutil is present only for linux64?


And wow, UniDep looks awesome!!! It would be great if we could collaborate. Apart from getting fresh eyes on the codebase to clean up stuff like this, I really wish that conda-lock could output more intermediate representations along the lines of your requirements.yaml.

@maresb maresb merged commit 0481cc6 into conda:main Dec 1, 2023
8 checks passed
@maresb
Copy link
Contributor

maresb commented Dec 1, 2023

Thanks @basnijholt!!!

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.

2 participants