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

Avoid issues related with pins persistence #355

Merged
merged 4 commits into from
Nov 6, 2023
Merged

Avoid issues related with pins persistence #355

merged 4 commits into from
Nov 6, 2023

Conversation

jaimergp
Copy link
Contributor

@jaimergp jaimergp commented Nov 3, 2023

Description

When the libmambapy.Solver calls .add_pin(), it injects some special packages in the installed pool / repo (see thread). We keep the same repo across attempts in our retry loop, so we only need to / should invoke that once.

That seems to fix #354, but let's see if another problems arise in the test suite.

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 3, 2023
n_pins = 0
tasks = self._specs_to_tasks(in_state, out_state)
for (task_name, task_type), specs in tasks.items():
log.debug("Adding task %s with specs %s", task_name, specs)
if task_name == "ADD_PIN":
if task_name == "ADD_PIN" and attempt == 1:
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense given the Matrix discussion linked in the OP.

If it doesn't complicate things too much, would it make more sense to add an is_initial_attempt: bool (or just attempt: int) parameter to _specs_to_tasks and have the ADD_PIN task simply not created?
To me, that would conceptually make more sense since creating a "task" that will be ignored otherwise is awkward ;).

Copy link
Member

Choose a reason for hiding this comment

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

(Or the tasks could be created beforehand and split into initial and recurring ones; but that might need more restructuring which might be overkill for this single special-handling case for now.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I get your point, but here's my rationale:

IMO the limitation that solver.add_pins() has persistent effects in the pool is an implementation detail of the libmamba library. specs_to_tasks aims to be as unaware of those implementation details as possible. It's not that we shouldn't pin in the 2nd iteration and following ones; we do want, but there's a technical limitation. I'd like specs_to_tasks to reflect that logic, hence why I ignore it here.

It could be cleaner, but we are going to remove a lot of code from the `state, and I plan to include some cleanup in the solver module.

I hope it makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

How many attempts would be typical?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends on the environment. Hopefully the first one is enough. But with old ones with many war scars, it can take a few. We fast-track some cases, like changing Python version, because otherwise we'd need for the solver to realize all the Python-depending packages are conflicting.

Copy link
Member

@mbargull mbargull left a comment

Choose a reason for hiding this comment

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

LGTM!
Confirmed working locally as well as via the CI tests.
(Test failures are unrelated connection errors;)

@jaimergp
Copy link
Contributor Author

jaimergp commented Nov 6, 2023

Thanks for the checks @mbargull!

Copy link
Contributor

@dholth dholth left a comment

Choose a reason for hiding this comment

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

LGTM

@jezdez jezdez merged commit 671b9c1 into main Nov 6, 2023
@jezdez jezdez deleted the fix-354 branch November 6, 2023 19:46
@github-actions github-actions bot added the locked [bot] locked due to inactivity label Nov 6, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Unable to downgrade (python) if (unrelated) packages are pinned and some package needs to be removed
5 participants