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

Prevent channel/dependency reordering when rendering the environment #203

Merged
merged 6 commits into from
Feb 21, 2024

Conversation

wolny
Copy link
Contributor

@wolny wolny commented Feb 19, 2024

Keeps the order of channels/pip dependencies as defined in environment.devenv.yml (fixes #117).

In the current version the rendering of the following:

name: test-env

channels:
  - pytorch
  - nvidia
  - conda-forge

dependencies:
  - pytorch
  - numpy
  - scikit-image
  - pip:
    - "-e c-package"
    - "-e b-package"
    - "-e D-package"

is incorrect:

# generated by conda-devenv, do not modify and do not commit to VCS
channels:
- pytorch
- nvidia
- conda-forge
dependencies:
- numpy
- pytorch
- scikit-image
- pip:
  - -e D-package
  - -e b-package
  - -e c-package
environment: {}
name: test-env

This PR prevents the reordering and generate the correct outcome:

# generated by conda-devenv, do not modify and do not commit to VCS
channels:
- pytorch
- nvidia
- conda-forge
dependencies:
- pytorch
- numpy
- scikit-image
- pip:
  - -e c-package
  - -e b-package
  - -e D-package
environment: {}
name: test-env

@wolny wolny changed the title Prevent channel/dependency reordering when rendering final environment Prevent channel/dependency reordering when rendering the environment Feb 19, 2024
@nicoddemus
Copy link
Member

Thanks @wolny,

Can you please come up with a test for this, to avoid a regression in the future? Thanks

@sdvillal
Copy link

Testing should include screaming out loud when there are conflicting orderings of channels or pip dependencies. We probably should allow opting-in for this to happen and give a best order when this would not matter (e.g., when using newer pip versions with dependency resolution or when disabling channel priority in conda).

@wolny
Copy link
Contributor Author

wolny commented Feb 20, 2024

Hi @nicoddemus, I've added a single test which checks the ordering of channels and pip dependencies. Cheers!

@nicoddemus
Copy link
Member

Question, does this keeps the order for all dependencies, or just channels/pip dependencies?

@wolny
Copy link
Contributor Author

wolny commented Feb 20, 2024

this change in fact prevents reordering of dependencies: https://github.com/ESSS/conda-devenv/blob/master/src/conda_devenv/devenv.py#L500 this line was responsible for reordering dependencies and I've change the merge_dependencies_version_specifications to prevent this. Channel ordering seems to be preserved in the current version, but since it's more important to prevent channel reordering I've added a test for it as well.

@nicoddemus
Copy link
Member

I see, thanks for clarifying.

Technically we should not need to preserve the order of the normal/conda dependencies, and sorting them actually has the benefit of making the diff smaller in case someone changes the order in their devenv files for any reason -- but anyways the generated environment.yml file is not supposed to be committed, so it really it does not matter in the end.

Thanks for the PR!

I will leave this open for a few days to given others the chance to review it.

@wolny
Copy link
Contributor Author

wolny commented Feb 20, 2024

@nicoddemus yes, the order of dependencies does not matter for conda, since it computes the resolution on the whole set of dependencies. However the order of channels and pip dependencies does matter as mentioned in #117 and should be preserved. Channel ordering is already preserved by conda-devenv (which was the main concern), but the pip dependency ordering is not and this PR fixes it.

CHANGELOG.rst Outdated Show resolved Hide resolved
@prusse-martin
Copy link
Member

Thank you.

@nicoddemus nicoddemus merged commit 55276d6 into ESSS:master Feb 21, 2024
7 checks passed
@wolny wolny deleted the fix-deps-ordering branch February 21, 2024 15:17
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.

Channel order should be respected (and maybe also pip dependency order)
4 participants