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

Use callback=reset_context in conda.plan #13357

Merged
merged 4 commits into from
Nov 28, 2023

Conversation

jaimergp
Copy link
Contributor

@jaimergp jaimergp commented Nov 22, 2023

Description

Thanks to all the research done by @mbargull:

We need to use reset_context instead of reset_context_default to prevent he latter from clearing configurations coming from .condarc files (see conda/conda-libmamba-solver#393 for details). We also need callback instead of stack_callback, because:

  1. conda.base.context.ContextStack is broken
    (pop/replace don't call self.apply).
  2. conda.plan is the only consumer of ContextStack
    => we know nothing else adds to the stack => no need for it.
  3. We know always use default search_path => no need for distinctions.

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Nov 22, 2023
jaimergp added a commit to jaimergp/conda-build that referenced this pull request Nov 22, 2023
@mbargull
Copy link
Member

Another thing I noticed was that SubdirData._cache_ does not use add_pip_as_python_dependency as a key.
We should add it here https://github.com/conda/conda/blob/23.10.0/conda/core/subdir_data.py#L77 .
Plus, the SubdirData.__init__ should then store the value of context.add_pip_as_python_dependency as an instance member (since loading of the actual data is not done during __init__ immediately, i.e., the value from context.add_pip_as_python_dependency could change between calls to __init__ and the value's actual usage in _process_raw_repodata afterwards.)

Because:
1. conda.base.context.ContextStack is broken
    (pop/replace don't call self.apply).
2. conda.plan is the only consumer of ContextStack
    => we know nothing else adds to the stack => no need for it.
3. We know always use default search_path => no need for distinctions.

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
@jaimergp
Copy link
Contributor Author

Nothing seems to be breaking. Errors are due to #13360.

@jaimergp jaimergp changed the title use stack_context, not stack_context_default Use callback=reset_context in conda.plan Nov 25, 2023
@mbargull mbargull marked this pull request as ready for review November 26, 2023 12:53
@mbargull mbargull requested a review from a team as a code owner November 26, 2023 12:53
@mbargull mbargull marked this pull request as draft November 27, 2023 09:46
mbargull added a commit to jaimergp/conda that referenced this pull request Nov 27, 2023
Dependencies of python are altered acc. to add_pip_as_python_dependency.
In case that configuration value is changed at runtime (currently only
observed in conda-build's tests), SubdirData._cache_ gets invalid.
The offline cache on disk already considers the option, such that the
changes here make runtime behavior consistent with the offline cache.

refs:
- conda#13357 (comment)
- conda/conda-build#5083 (comment)

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
mbargull added a commit to jaimergp/conda that referenced this pull request Nov 27, 2023
Dependencies of python are altered acc. to add_pip_as_python_dependency.
In case that configuration value is changed at runtime (currently only
observed in conda-build's tests), SubdirData._cache_ gets invalid.
The offline cache on disk already considers the option, such that the
changes here make runtime behavior consistent with the offline cache.

refs:
- conda#13357 (comment)
- conda/conda-build#5083 (comment)

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
@mbargull mbargull force-pushed the context-no-default branch 2 times, most recently from 2b308b1 to fa75051 Compare November 27, 2023 11:09
@mbargull
Copy link
Member

(I had this commit 2b308b1 in here too

Fix ContextStack.pop/replace reset last used args

N.B: After the recent change to conda.plan.install_actions to not use
stack_context_default anymore, conda.base.ContextStack and related
functions are only used for test code.
As such, that code may be marked as deprecated and moved to the tests
entirely.

to call ContextStack's apply in pop/replace.
That broke a lot of tests immediately (probably esp. due to changing replace).
So, let's not change that code for now :D.
But the nota bene still applies: We may want to mark it as deprecated and move it to the tests.)

@mbargull
Copy link
Member

mbargull commented Nov 27, 2023

Tests fail because they seem broken.
They load the index with add_pip_as_python_dependency=False through conda.testing.helpers but don't set context.add_pip_as_python_dependency accordingly.
Plus there's a comment at https://github.com/conda/conda/blob/23.10.0/conda/testing/helpers.py#L600

    # We need CONDA_ADD_PIP_AS_PYTHON_DEPENDENCY=false here again (it's also in
    # get_index_r_*) to cover solver logics that need to load from disk instead of
    # hitting the SubdirData cache

that kind of foreshadows more issues when fixing the tests...

conda/testing/helpers.py Outdated Show resolved Hide resolved
@mbargull
Copy link
Member

Okay, the SubdirData._cache_ add_pip-awareness needs more investigation.
I'm splitting this off into a separate PR so we can at least go ahead with the conda.plan.install_actions change which on its own should unblock (at least many, if not all) conda-libmamba-solver+conda-build issues.

mbargull added a commit to mbargull/conda that referenced this pull request Nov 27, 2023
Dependencies of python are altered acc. to add_pip_as_python_dependency.
In case that configuration value is changed at runtime (currently only
observed in conda-build's tests), SubdirData._cache_ gets invalid.
The offline cache on disk already considers the option, such that the
changes here make runtime behavior consistent with the offline cache.

refs:
- conda#13357 (comment)
- conda/conda-build#5083 (comment)

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
@mbargull
Copy link
Member

mbargull commented Nov 27, 2023

Opened gh-13365 for the SubdirData._cache_ changes.
It would be good to get those in too since they are related to the index issues we are addressing here.
However, the issues with SubdirData._cache_ will likely not be encountered during normal conda/conda-build operation (add_pip_as_python_dependency should normally not change).

I'm adding this PR to the next milestone since it is effectively a blocker for conda-libmamba-solver (and thus conda>=23.10 with its new solver default) adoption in common conda-build uses (e.g., for conda-forge).

@mbargull mbargull marked this pull request as ready for review November 27, 2023 13:48
@mbargull mbargull added this to the 23.11.0 milestone Nov 27, 2023
@mbargull
Copy link
Member

I'm adding this PR to the next milestone since it is effectively a blocker for conda-libmamba-solver (and thus conda>=23.10 with its new solver default) adoption in common conda-build uses (e.g., for conda-forge).

And a note for other reviewers: The changes here are pretty much confined to conda-build since conda.plan.install_actions is not used within conda itself. Meaning, this is high impact for conda-build but low impact for conda.

@jaimergp
Copy link
Contributor Author

I think it's time to mark tests/core/test_solve.py::test_conda_downgrade[libmamba] as xfailable (strict=False). See #13367.

@kenodegard kenodegard merged commit 7823633 into conda:main Nov 28, 2023
67 checks passed
@kenodegard kenodegard mentioned this pull request Dec 1, 2023
44 tasks
mbargull added a commit to mbargull/conda-build that referenced this pull request Jan 19, 2024
Port of fix from conda/conda#13357 .

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
mbargull added a commit to mbargull/conda-build that referenced this pull request Jan 19, 2024
Port of fix from conda/conda#13357 .

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Archived in project
4 participants