✨ Add daemon version drift warning#7318
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7318 +/- ##
==========================================
+ Coverage 80.08% 80.15% +0.07%
==========================================
Files 576 577 +1
Lines 45280 45475 +195
==========================================
+ Hits 36259 36445 +186
- Misses 9021 9030 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
eb8eafe to
c78afcb
Compare
Record installed package versions when the daemon starts or restarts, and warn in if current versions differ from those the daemon was started with. This helps catch subtle bugs caused by upgrading aiida-core or plugins without restarting the daemon. Apply pre-commit formatting fixes during the replay so the commit passes the repository linting workflow on its own.
Read VCS commit identifiers from importlib distribution metadata instead of calling git in the source checkout. This keeps the daemon version snapshot based on installation metadata and still distinguishes packages installed from specific VCS commits. Editable local installs now fall back to the package version alone, which matches the intended scope of the feature. Apply pre-commit formatting fixes during the replay so the commit passes the repository linting workflow on its own.
c78afcb to
bd2cb3e
Compare
Record the package snapshot used when the daemon starts and show a `verdi status` warning when the current environment no longer matches it. This covers VCS installs via package metadata and editable installs via normalized source paths.
bd2cb3e to
bff9297
Compare
Record the package snapshot used when the daemon starts and show a `verdi status` warning when the current environment no longer matches it. This covers VCS installs via package metadata and editable installs via normalized source paths.
bff9297 to
c2908db
Compare
Record the package snapshot used when the daemon starts and show a `verdi status` warning when the current environment no longer matches it. This covers VCS installs via package metadata and editable installs via normalized source paths.
c2908db to
c185fc7
Compare
Record the package snapshot used when the daemon starts and show a `verdi status` warning when the current environment no longer matches it. This covers VCS installs via package metadata and editable installs via normalized source paths.
c185fc7 to
ab0f6bf
Compare
Record the package snapshot used when the daemon starts and show a `verdi status` warning when the current environment no longer matches it. This covers VCS installs via package metadata and editable installs via normalized source paths.
ab0f6bf to
81d4cd8
Compare
GeigerJ2
left a comment
There was a problem hiding this comment.
OK, posting my pending comments already, as I need to sign off for today. Will continue tomorrow.
| return self._config.filepaths(self.profile)['daemon']['pid'] | ||
|
|
||
| @property | ||
| def daemon_version_file(self) -> str: |
There was a problem hiding this comment.
Naming: the key in filepaths is version_info, the file extension is .version, and this property is daemon_version_file. Three terms for the same thing. Pick one — I'd suggest package_snapshot everywhere, since this records package state, not "the AiiDA version".
| @staticmethod | ||
| def get_package_version_snapshot() -> PackageVersionSnapshot: | ||
| """Return version information for installed packages that provide ``aiida.*`` entry points. | ||
|
|
||
| For packages installed from a git repository through a VCS URL, the | ||
| version string includes the recorded commit hash from the distribution | ||
| metadata. Editable installs from a local path record their normalized | ||
| source path. | ||
| """ | ||
| from aiida.plugins.entry_point import ENTRY_POINT_GROUP_PREFIX, eps | ||
|
|
||
| versions: PackageVersionSnapshot = {} | ||
| for ep in eps(): | ||
| if ep.group.startswith(ENTRY_POINT_GROUP_PREFIX) and ep.dist: | ||
| if ep.dist.name not in versions: | ||
| version_info: PackageVersionInfo = {'version': ep.dist.version} | ||
| direct_url_data = _get_dist_direct_url_data(ep.dist) | ||
| commit = _get_dist_commit_hash(ep.dist, direct_url_data) | ||
| editable_path = _get_dist_editable_path(ep.dist, direct_url_data) | ||
|
|
||
| if commit: | ||
| version_info['version'] = f'{ep.dist.version}+{commit[:8]}' | ||
|
|
||
| if editable_path: | ||
| version_info['editable_path'] = editable_path | ||
|
|
||
| versions[ep.dist.name] = version_info | ||
| return versions |
There was a problem hiding this comment.
Two structural concerns:
-
Misplaced as a method on
DaemonClient. This@staticmethoddoes not touch any instance state — it inspects the local Python environment by walkingeps()and readingdirect_url.jsonfrom eachdist. Nothing about it is daemon-y; the daemon is just one consumer that happens to persist the snapshot to a file. The mixed call sites prove the awkwardness:client.get_package_version_snapshot()in_write_version_filevsDaemonClient.get_package_version_snapshot()incmd_status.py:180/cmd_daemon.py:158. Best home isaiida.plugins.entry_pointas a module-level function (e.g.get_aiida_package_versions()) — that module already ownseps(),ENTRY_POINT_GROUP_PREFIX, and the surrounding entry-point introspection. The same applies to_validate_package_version_snapshot(:636) and the_get_dist_*helpers (:66-149) — none of them touchDaemonClientstate. ThePackageVersionInfoTypedDict andPackageVersionSnapshotalias move there too, all still private. The only things that should stay onDaemonClientare the two instance methods that own the daemon-specific file path:_write_version_fileandget_daemon_version_info— both just call the new module-level functions. -
Commit hash folded into the version string.
version_info['version'] = f'{ep.dist.version}+{commit[:8]}'(line 625-628) packs two semantically different things into one string. For a VCS install we store{'version': '2.8.0.post0+abcdef12'}instead of{'version': '2.8.0.post0', 'commit': 'abcdef12'}. Few downsides:- Hard to read.
format_package_state_change_linesdoes a string-equality compare onversion, so commit-only drift shows up asChanged packages: aiida-core (2.8.0.post0+abc12345 -> 2.8.0.post0+def67890). The user has to mentally parse the+suffix to realise the version didn't actually move — only the commit did. (Same readability issue you get (at least I get... ^^) from staring atpip install git+...@<sha>URLs.) With a separatecommitfield the formatter could sayChanged packages: aiida-core (commit abc12345 -> def67890). - Hijacks PEP 440 local-version syntax. PEP 440 allows
+local, so it's not technically invalid, but a future reader sees2.8.0.post0+abc12345and can't tell whetherabc12345is a commit, a build number, a vendor tag, etc. A typedcommit: strfield is self-documenting. - Loses filterability. A future "warn only on actual version bumps, ignore commit-only drift" check would have to parse the
+suffix back out. With separate fields it's justdaemon['version'] != current['version'].
Suggested shape:
version_info: PackageVersionInfo = {'version': ep.dist.version} if commit: version_info['commit'] = commit[:8]
plus a
commit: NotRequired[str]field onPackageVersionInfo, andformat_package_version_infobecomes aware of it the same way it currently is ofeditable_path. - Hard to read.
|
Sorry for the very pedantic review here @agoscinski, that's the character I gave my review agent :D All the comment I posted were flagged during the review. I already filtered / adapted where I think it makes sense. Your call if you agree, or if they are too nitpicky for the PR. But, why not post them, if I have them already :) Talking about nitpicks, here are a few more of them:
That's it. Will wait for the updates, then check again. |
Record the package snapshot used when the daemon starts and show a `verdi status` warning when the current environment no longer matches it. This covers VCS installs via package metadata and editable installs via normalized source paths.
bc1847d to
7f74488
Compare
Record a snapshot of installed package versions when the daemon starts or restarts, and warn on `verdi status` / `verdi daemon status` when the current environment no longer matches. This covers version changes, added/removed plugins, editable install path changes, and VCS installs via commit hash from distribution metadata. Implementation details: - A JSON file (`.package_snapshot`) is written by `DaemonClient` on `start_daemon` (after `_await_condition`) and `restart_daemon` (gated on `response.status == 'ok'`), and cleaned up on `stop_daemon`. The file contains a `PackageVersionSnapshot` mapping package names to version and optional editable path. - `get_package_version_snapshot()` walks all `aiida.*` entry points and reads `direct_url.json` metadata (PEP 610) to detect editable installs and VCS commit hashes. - `get_daemon_package_drift_lines()` in `cmdline/utils/daemon.py` compares the daemon-recorded snapshot against the live one and returns categorized mismatch lines (Changed/Added/Removed). Both snapshots are validated before comparison. - New unit tests in `tests/cmdline/utils/test_daemon.py` pin the formatter contract without CLI startup overhead. Integration tests in `test_status.py` and `test_daemon.py` verify the warning surfaces correctly.
c5c7e21 to
2fe4fba
Compare
Record a snapshot of installed package versions when the daemon starts or restarts, and warn on `verdi status` / `verdi daemon status` when the current environment no longer matches. This covers version changes, added/removed plugins, editable install path changes, and VCS installs via commit hash from distribution metadata. Implementation details: - A JSON file (`.package_snapshot`) is written by `DaemonClient` on `start_daemon` (after `_await_condition`) and `restart_daemon` (gated on `response.status == 'ok'`), and cleaned up on `stop_daemon`. The file contains a `PackageVersionSnapshot` mapping package names to version and optional editable path. - `get_package_version_snapshot()` walks all `aiida.*` entry points and reads `direct_url.json` metadata (PEP 610) to detect editable installs and VCS commit hashes. - `get_daemon_package_drift_lines()` in `cmdline/utils/daemon.py` compares the daemon-recorded snapshot against the live one and returns categorized mismatch lines (Changed/Added/Removed). Both snapshots are validated before comparison. - New unit tests in `tests/cmdline/utils/test_daemon.py` pin the formatter contract without CLI startup overhead. Integration tests in `test_status.py` and `test_daemon.py` verify the warning surfaces correctly.
2fe4fba to
300080b
Compare
|
I should have applied all suggestions, even the nitpicky once.
I will merge it now if CI passes (besides flaky tests) because I will anyway not be around to adapt any further review feedback. Its easier for me to make another support branch if the PRs are merged into main (otherwise I have to consider errors that are due to merge conflicts). |
|
I checket the CI. The errors are |
|
Just adding the review items I think are sensible, and that didn't get adressed here, for possible follow-up:
|
Record a snapshot of installed package versions when the daemon starts or restarts, and warn on `verdi status` / `verdi daemon status` when the current environment no longer matches. This covers version changes, added/removed plugins, editable install path changes, and VCS installs via commit hash from distribution metadata. Implementation details: - A JSON file (`.package_snapshot`) is written by `DaemonClient` on `start_daemon` (after `_await_condition`) and `restart_daemon` (gated on `response.status == 'ok'`), and cleaned up on `stop_daemon`. The file contains a `PackageVersionSnapshot` mapping package names to version and optional editable path. - `get_package_version_snapshot()` walks all `aiida.*` entry points and reads `direct_url.json` metadata (PEP 610) to detect editable installs and VCS commit hashes. - `get_daemon_package_drift_lines()` in `cmdline/utils/daemon.py` compares the daemon-recorded snapshot against the live one and returns categorized mismatch lines (Changed/Added/Removed). Both snapshots are validated before comparison. - New unit tests in `tests/cmdline/utils/test_daemon.py` pin the formatter contract without CLI startup overhead. Integration tests in `test_status.py` and `test_daemon.py` verify the warning surfaces correctly. Reviewed-by: Julian Geiger <julian.geiger@psi.ch>
Record installed package versions when the daemon starts or restarts in a file, and warn in
verdi statusandverdi daemon statusif current versions differ from those the daemon was started with. This helps catch subtle bugs caused by upgrading aiida-core or plugins without restarting the daemon.After playing with different possibilities I only used the information that is available in
direct_url.jsonthat follows the specification from https://packaging.python.org/en/latest/specifications/direct-url-data-structure/ so the approach is robust. In contrastpip freezedoes for example retrieves more information by for example in case of a editable mode install checking the local directory for the git hash. This allows to also detect local changes in the editable installation but would introduce extra logic that tries to retrieve the HEAD hash from the directory. I don't think this feature is worth the complexity.In the examples below you see that I added a fish-like compression of the directory
~/c/a/w/f/new-checkoutI am not sure if this is the best way, but including the whole directory is quite verbose and I kind of like the way fish does it.Note that there can be still a change in the aiida-core dependencies and this will not be tracked. To detect this it would require a comparison of the complete dependency tree which would make verdi status way too slow.
Note that by using
ep.dist.versionlocal changes inaiida.__version__are not detected for an editable installation (only when newly pip installed). I think that also is consistent with the philosophy that we do not track local changes in editable mode.Below the agent summary, its quite comprehensive I would say.
Description
This PR adds daemon package-state tracking and surfaces a warning in
both
verdi statusandverdi daemon statuswhen the currentlyinstalled AiiDA packages no longer match the package state the daemon
was started with.
The goal is to make daemon/environment drift visible before it turns
into confusing runtime behavior after:
aiida-coreThe warning compares the package snapshot recorded at daemon startup
against the current package snapshot.
What is tracked:
For editable installs, this PR intentionally compares normalized source
paths, not the checkout's current git
HEAD. Editable checkouts canchange without a commit change, and package metadata for local
editables does not reliably expose VCS commit information.
Examples
Version or editable-path mismatch:
Plugin removed after daemon start:
Plugin added after daemon start:
The same mismatch is also surfaced through
verdi daemon status:Implementation Notes
direct_url.json/ distribution originmetadata when available
verdi statusandverdi daemon statusverdi statusoutput to a singledaemon:line block