Consider installed packages in solve - #101
Conversation
86b12e5 to
a648bb0
Compare
d3ab698 to
e38f25d
Compare
16c9ca6 to
264b168
Compare
| count += info.repo.record_count(self._package_format) | ||
| return count | ||
|
|
||
| def get_info(self, key: str) -> _ChannelRepoInfo: |
There was a problem hiding this comment.
This is not used anywhere.
| assert "python" in unlink_names.intersection(link_names) | ||
|
|
||
|
|
||
| def test_installed_packages_included_in_solver( |
There was a problem hiding this comment.
Hey @soapy1, your current implementation is consistent with the other tests above which is great, but do you think it is possible to use the TmpChannelFixture and the mamba_repo here to avoid network calls? This would speed up the test a ton and I don't think it would give up much for test coverage.
jaimergp
left a comment
There was a problem hiding this comment.
I need to take a deeper look but I don't think we need this. The rattler APIs are different than libmamba's. Please don't merge until I review next week.
|
@jaimergp thanks for reviewing this! I'm definitely not super confident that this is the right approach. Sharing some notes: I was looking at some alternative ways at passing information related to what is already installed in the conda prefix to the rattler solver.
digging more into py-rattler
|
|
@soapy1 I actually tried this, but the overhead of marshaling the data from One thing we could improve, though, is to allow the For what it's worth, at least on the Rust side, Even better would be to let |
ah, I see. I was under the impression that gateway already had this functionality.
I really like this idea, trying it out! |
|
Trying out using the This is just a rough demo, I think there is a more elegant way to structure this change in c-r-s. I did some initial benchmarking and it looks like this is not introducing new performance issues. edit: below are some results from a quick test (ref: #42 (comment)) this prusing the alternate demoref: conda/rattler#2627 and https://github.com/conda/conda-rattler-solver/compare/main...soapy1:add-installed-records-to-solve-pool-2?expand=1 on main |
ryanskeith
left a comment
There was a problem hiding this comment.
Overall looks good to me. I have a small suggestion for optimization.
| if record.subdir not in self._subdirs: | ||
| continue | ||
| record_data = dict(record.dump()) | ||
| for field in ( |
jezdez
left a comment
There was a problem hiding this comment.
Thanks for working through this, Sophia. The issue is real, and the default reproduction now succeeds. I found two supported solver configurations where installed records still disappear as usable candidates. Both come from representing prefix records as ordinary channel repodata.
With strict channel priority, a package in the active channel excludes a compatible installed dependency from its removed origin channel. With CONDA_USE_ONLY_TAR_BZ2=true, an installed .conda record is filtered out even though keeping it requires no download.
I think the fix should keep the first solve unchanged, then add only an installed record named by a No candidates were found failure to locked_packages on the retry. Rattler inserts locked records directly into the candidate pool. This avoids channel priority and package format filtering while preserving normal update --all behavior for packages that do have current candidates. The missing-installed condition should remain separate from generic conflicts so we do not lock every relaxed conflict.
I left direct suggestions for the concrete channel and subdir bug in the current implementation, plus comments for the solver semantics and deterministic coverage. Those suggestions fix the metadata bug but not the central fallback semantics. The repoquery relaxation is unrelated and should move separately. I would keep this draft until the fallback is represented outside ordinary repodata.
| subdirs=subdirs, | ||
| repodata_fn=self._repodata_fn, | ||
| pkgs_dirs=context.pkgs_dirs if context.offline else (), | ||
| installed_records=(*in_state.installed.values(),), |
There was a problem hiding this comment.
Passing every installed record into the ordinary repodata pool does not give these records the fallback semantics we need. I reproduced two failures on this head:
- With strict channel priority, an active channel containing
foo=1excludes installedfoo=2from its removed origin channel, even when installedbarrequiresfoo>=2. - With
CONDA_USE_ONLY_TAR_BZ2=true, installed.condarecords are filtered out of this pool even though they require no download.
Could we keep the initial solve unchanged, carry a separate missing_installed: set[str] across retries, add a name to it only in the existing No candidates were found branch when the spec matches the installed record, and add only those records to locked_packages in _collect_specs_main? Rattler inserts locked records directly into the candidate pool, so that path succeeds under both settings without locking every package during a normal update --all solve. Keeping this separate from out_state.conflicts also preserves the existing no-history transitive-drop behavior.
| "--all", | ||
| f"--prefix={prefix}", | ||
| "--override-channels", | ||
| "--channel=conda-canary", |
There was a problem hiding this comment.
This depends on live defaults and conda-canary, then only checks the return code and stderr. Channel contents can change and make the test pass without exercising the fallback. Could we use local repodata and assert that the installed record and its URL remain unchanged? A deterministic version can create test-package from tests/data/mamba_repo, switch to a local empty channel, and assert the no-op result. Please also cover the two cases that currently fail on this head: strict priority with an incompatible same-name candidate in the active channel, and CONDA_USE_ONLY_TAR_BZ2=true with an installed .conda record.
| # Upstream: https://github.com/mamba-org/mamba/issues/4346 | ||
| # Same carve-out pattern in conda-libmamba-solver tests/test_repoquery.py | ||
| # (https://github.com/conda/conda-libmamba-solver/pull/964). | ||
| assert python |
There was a problem hiding this comment.
This network-sensitive repoquery relaxation is unrelated to making installed records available during a solve. Could we move it to a separate PR so this change keeps a focused failure surface?
Co-authored-by: Ken Odegard <kodegard@anaconda.com>
Co-authored-by: Jannis Leidel <jannis@leidel.info>
Co-authored-by: Jannis Leidel <jannis@leidel.info>
24032ab to
efb4bca
Compare
Description
This change allows packages installed in the environment to be considered in the solve by including them in them in the
RattlerIndexHelper. We do not include virtual packages in the set of installed packages.With this change, removing a channel from an environment and running an update no longer produces an Unsatisfiable error.
fixes #88
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?