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

Fail gracefully if offline and no cache is available #323

Merged
merged 9 commits into from
Oct 19, 2023
Merged

Conversation

jaimergp
Copy link
Contributor

@jaimergp jaimergp commented Oct 13, 2023

Description

Needed for conda/conda#12984

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 Oct 13, 2023
Copy link
Contributor

@costrouc costrouc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and can confirm in pytest tests/test_create.py -k test_offline_with_empty_index_cache\[libmamba\] that the test continues but still does eventually fail. The error messages though are more helpful in debugging.

@costrouc
Copy link
Contributor

costrouc commented Oct 16, 2023

I will be investigating the tests that fail and why it is happening reliably on windows. https://github.com/conda/conda-libmamba-solver/actions/runs/6513586579/job/17735028419?pr=323#step:9:935

@costrouc
Copy link
Contributor

costrouc commented Oct 16, 2023

Triggering a rerun and seeing what happens. I ran this test locally on a windows vm and wasn't able to reproduce.

@costrouc
Copy link
Contributor

costrouc commented Oct 16, 2023

Spent a long time trying to debug the windows issue. I see a difference of assert PATH0.lower() == PATH.lower() in left side having /usr/bin in the path and the other has /bin in the path. On line https://github.com/conda/conda/blob/e784a57001b129804f17e10cce4cd31400be8103/tests/test_activate.py#L2696. I'm wondering how we test this. @jaimergp any ideas?

@jaimergp
Copy link
Contributor Author

I'd say we skip it for now because it's not solver related at all. Can you deselect in the pytest collection plugin? Thanks!

@jaimergp
Copy link
Contributor Author

tests/conda_env/test_cli.py::test_update_env_no_action_json_output errors are legit but caused by a unique situation. This is the gist:

  • Having an env.yml file with python=3, pip and pip: click, we should be able to create it with conda env create.
  • A subsequent call to conda env update should do nothing because we just installed everything.

Makes sense, but this is what we observe in this error:

  1. conda env create creates an environment with python=3.11 and pip=23.3. It prioritizes having a greater pip version than a Python one, possibly because it comes earlier in the sorting (i beats y).
  2. However, conda env update imposes python=3 again (not just python!), which is interpreted as "update python". This time, the solver tries to satisfy the petition more aggressively, even allowing a pip downgrade this time, because the alternative is to say "we are ok".

We didn't see this error earlier because the packaging situation is unique: defaults has Python 3.12 now, but only pip 23.2 is available for that version. The most recent 23.3 is only available for Python 3.11. I'm assuming once the new pip is available for 3.12 this will pass, but the currently observed behavior is a bit surprising and might deserve a fix.

@jaimergp
Copy link
Contributor Author

Note that changing the environment.yml file to the following makes everything pass as expected:

ENVIRONMENT_PYTHON_PIP_CLICK = f"""
name: {TEST_ENV_NAME_1}
dependencies:
- - python=3
+ - python
  - pip
  - pip:
    - click
channels:
  - defaults
"""

@jaimergp
Copy link
Contributor Author

This alternative change in solver.py also makes things pass. MatchSpec.merge sorts everything by str, and we used to keep that:

        self.specs_to_add = IndexedSet(MatchSpec.merge(s for s in fixed_specs))

However, if we prio Python, it passes:

        resorted_specs = []
        for spec in MatchSpec.merge(s for s in fixed_specs):
            if spec.name == "python":
                resorted_specs.insert(0, spec)
            else:
                resorted_specs.append(spec)
        self.specs_to_add = IndexedSet(resorted_specs)

@jaimergp jaimergp merged commit b29cea4 into main Oct 19, 2023
73 checks passed
@jaimergp jaimergp deleted the offline-no-cache branch October 19, 2023 14:47
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
Development

Successfully merging this pull request may close these issues.

xfail filters outdated for some unit tests now (libmamba 1.5.2 not covered)
4 participants