🔧 Enable and apply ruff pyupgrade - #7254
Conversation
492b5ca to
bc958e0
Compare
|
I would do this only after @edan-bainglass merges his big pydantic PR, otherwise I'd worry he would suffer greatly from conflicts. Not sure if there are other big PRs to be merged first? Also, I'd recommend upgrading ruff first in a separate PR. We're on quite an old version and the new versions had fixes for UP rules (and better/more autofixes). |
|
All very good points, @danielhollas, agree with all! Not gonna invest more time here for now. Just wanted to play around with it :) will get back here after @edan-bainglass's PR is merged |
|
Consider also dropping the |
bc958e0 to
c914180
Compare
Co-authored-by: Alexander Goscinski <alex.go@posteo.de>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR performs a broad, codebase-wide modernization of Python type annotations, converting ChangesCodebase-wide typing modernization
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7254 +/- ##
==========================================
+ Coverage 80.60% 80.61% +0.01%
==========================================
Files 580 580
Lines 46718 46722 +4
==========================================
+ Hits 37653 37659 +6
+ Misses 9065 9063 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Probably its easier for merge conflicts to just do apply this to ones local changes |
|
I am not sure I would run pyupgrade, because there might be differences between ruff and pyupgrades implementation. Instead you could use ruff with a |
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aiida/transports/transport.py (1)
321-331: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPreserve shell-safe escaping for
remotedir. Interpolatingremotedirasf"'{remotedir}'"breaks on paths containing'and can alter the shell command generated bygotocomputer_command. Use the existing shell-escaping helper orshlex.quotefor every interpolation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/transports/transport.py` around lines 321 - 331, Update _gotocomputer_string to shell-escape remotedir using the existing escaping helper or shlex.quote before interpolating it into the generated command. Reuse the escaped value for every remotedir occurrence, including the directory test and error message, while preserving the existing command flow.
🧹 Nitpick comments (8)
tests/utils/archives.py (1)
81-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the Sphinx-style docstring.
Document the
list[dict]return value and theValueErrorraised for unsupported archive formats.Suggested update
:param names: the files to retrieve - + :return: The decoded JSON objects in the requested order. + :raises ValueError: If the path is not a supported archive format.As per coding guidelines, Python docstrings must use Sphinx-style
:param:,:return:, and:raises:fields, with types kept in annotations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/archives.py` around lines 81 - 88, Complete the Sphinx-style docstring for read_json_files by adding a :return: description for the list[dict] result and a :raises ValueError: entry covering unsupported archive formats, while preserving the existing parameter documentation and type annotations.Source: Coding guidelines
src/aiida/orm/implementation/storage_backend.py (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
typing.ContextManagerleft un-modernized.
ContextManager(used at Line 232) is deprecated since Python 3.9 in favor ofcontextlib.AbstractContextManager, and is covered by Ruff'sUP035rule that this PR aims to fully apply. Consider migrating alongside the other typing changes in this PR.♻️ Suggested fix
-from typing import TYPE_CHECKING, Any, ContextManager, TypeVar +from contextlib import AbstractContextManager +from typing import TYPE_CHECKING, Any, TypeVarand update the usage at Line 232:
- def transaction(self) -> ContextManager[Any]: + def transaction(self) -> AbstractContextManager[Any]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/implementation/storage_backend.py` at line 16, Replace the deprecated typing.ContextManager import with contextlib.AbstractContextManager, then update the corresponding annotation in the implementation using ContextManager to reference AbstractContextManager while preserving its existing type parameters and behavior.src/aiida/transports/plugins/async_backend.py (1)
98-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the timeout type across the async backend contract.
AsyncSshTransport.exec_command_wait_asyncacceptsfloat | Noneand passes it torun, but the abstract backend and_AsyncSSHstill declareint | None, while_OpenSSHusesfloat | None. Align these annotations—preferably tofloat | None—to avoid a narrowed interface and possible mypy errors for fractional timeouts.This is based on the supplied
AsyncSshTransport.exec_command_wait_asynccaller insrc/aiida/transports/plugins/ssh_async.py, Lines 855-890.Also applies to: 264-264, 475-475, 698-698
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/transports/plugins/async_backend.py` at line 98, Align the timeout annotations across the async backend contract by changing the abstract backend run method and the `_AsyncSSH` implementation to accept `float | None`, matching `AsyncSshTransport.exec_command_wait_async` and `_OpenSSH`. Update all referenced `run` signatures while preserving their existing timeout behavior.src/aiida/engine/processes/ports.py (1)
261-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the accepted mapping interface.
The implementation explicitly accepts any
Mapping, not only concrete dictionaries. UseMapping[str, Any] | Nonefor the parameter while keeping the dictionary return type.Proposed fix
- def serialize(self, mapping: dict[str, Any] | None, breadcrumbs: Sequence[str] = ()) -> dict[str, Any] | None: + def serialize(self, mapping: Mapping[str, Any] | None, breadcrumbs: Sequence[str] = ()) -> dict[str, Any] | None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/engine/processes/ports.py` at line 261, Update the serialize method parameter annotation to accept Mapping[str, Any] | None instead of dict[str, Any] | None, while preserving its dict[str, Any] | None return annotation and implementation behavior.src/aiida/orm/nodes/data/remote/stash/folder.py (1)
80-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the element type in
source_list.The getter and setter currently widen the contract to
list | tuple, while the model and stash validation require sequences of string paths. Preferlist[str] | tuple[str, ...]so static consumers retain the element type.Proposed fix
- def source_list(self) -> list | tuple: + def source_list(self) -> list[str] | tuple[str, ...]: ... - def source_list(self, value: list | tuple): + def source_list(self, value: list[str] | tuple[str, ...]):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/nodes/data/remote/stash/folder.py` around lines 80 - 88, Update the source_list property getter and setter in the source_list definition to use list[str] | tuple[str, ...] instead of list | tuple, preserving the required string-path element type for static consumers while leaving the existing behavior unchanged.src/aiida/orm/nodes/process/process.py (1)
179-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse iterable unpacking for the tuple extension.
Ruff’s RUF005 warning applies here; unpacking preserves the same value and type without tuple concatenation.
Proposed refactor
-return super()._hash_ignored_attributes + ('metadata_inputs',) +return (*super()._hash_ignored_attributes, 'metadata_inputs')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/nodes/process/process.py` at line 179, Update the _hash_ignored_attributes property to extend the superclass tuple using iterable unpacking instead of tuple concatenation, preserving the existing metadata_inputs value and tuple result.Source: Linters/SAST tools
src/aiida/tools/_dumping/tracking.py (1)
14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
Iteratorforiter_by_type().Line 14 changes the import to
collections.abc.Generator, butiter_by_type()still uses the one-parameter form at Lines 383-387. Since it only yields values, useIterator[...]instead; the typing guidance recommendsIteratorfor ordinary generator functions and reservesGeneratorfor explicit send/return types. (docs.python.org)Proposed fix
-from collections.abc import Collection, Generator +from collections.abc import Collection, Iterator - ) -> Generator[ + ) -> Iterator[Please verify with the repository’s mypy target.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/tools/_dumping/tracking.py` around lines 14 - 18, Update the return annotation of iter_by_type() to use collections.abc.Iterator for its yielded values, and replace the Generator import if it is no longer used elsewhere in the module. Verify the change with the repository’s mypy target.src/aiida/tools/data/array/kpoints/legacy.py (1)
1088-1088: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAddress Ruff RUF005 and avoid unordered permutation output.
This changed line still uses list concatenation, which Ruff flags. It also relies on set iteration order for the two remaining axes. Prefer deterministic iterable unpacking:
Proposed fix
-permutation = [in_line_index] + list(set(range(3)) - {in_line_index}) +permutation = [in_line_index, *[index for index in range(3) if index != in_line_index]]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/tools/data/array/kpoints/legacy.py` at line 1088, Update the permutation construction near the affected k-point conversion logic to avoid list concatenation flagged by Ruff RUF005 and replace set-based iteration with a deterministic ordering for the two remaining axes. Preserve in_line_index as the first element and append the remaining axis indices via ordered iterable unpacking.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aiida/engine/daemon/execmanager.py`:
- Line 831: Update the retrieve_list annotation to allow nullable tuple depth by
changing its third tuple element from int to int | None, matching the existing
depth is None handling.
- Around line 20-24: Update the Mapping type check in _find_data_node to use the
imported MappingType alias, or import Mapping under the name used by the check,
so nested inputs no longer raise NameError.
In `@src/aiida/engine/processes/ports.py`:
- Around line 118-125: Update the serialize method’s return annotation to
reflect that it may return the original value when no serializer is configured,
when the input is None, or when it is already non-Data; use Any or appropriate
overloads while preserving the existing serialization behavior.
In `@src/aiida/engine/processes/process.py`:
- Line 293: Update the return annotation of get_provenance_inputs_iterator so
its yielded value type matches the actual input values produced by
self.inputs.items(), rather than InputPort | PortNamespace; keep the key type
and iterator structure unchanged, and express the type solely through the
annotation.
In `@src/aiida/orm/nodes/data/array/bands.py`:
- Line 124: In the tuple assignment beginning at bands, occupations, correct the
closing delimiter for the opened parenthesized expression near the end of the
generator expression from ] to ). Ensure the module parses and imports
successfully.
In `@src/aiida/orm/nodes/data/data.py`:
- Line 43: Update the mutable class attribute _export_format_replacements by
annotating it as ClassVar[dict[str, str]], and import ClassVar from typing if
needed. Keep the shared empty-dictionary configuration unchanged while resolving
Ruff RUF012.
In `@src/aiida/plugins/entry_point.py`:
- Around line 293-297: Update the Sphinx return descriptions for
get_entry_point_groups() and get_entry_point_from_class() to match their
annotated contracts: document a set of strings for the former and a two-element
tuple containing None values when no match is found for the latter. Change
documentation only and preserve the existing implementation.
In `@src/aiida/storage/psql_dos/orm/querybuilder/main.py`:
- Around line 901-905: Fix the modify_expansions parameter declaration by using
a type annotation for outer_to_inner_schema instead of assigning a default
generic alias; preserve its required-argument behavior and existing dict[str,
dict[str, str]] type.
In `@src/aiida/tools/_dumping/utils.py`:
- Line 391: Update get_directory_stats so an empty directory, identified by
latest_mtime_ts remaining zero, returns (None, 0) without accessing an
uninitialized dir_mtime; preserve the existing timestamp and size behavior for
non-empty directories.
In `@src/aiida/tools/archive/abstract.py`:
- Around line 81-84: Update the Sphinx docstring for bulk_insert so its
parameter tag uses rows instead of data, matching the method signature; leave
the existing type annotations and other documentation unchanged.
In `@src/aiida/tools/archive/implementations/sqlite_zip/main.py`:
- Around line 72-75: Update the migrate method docstring to replace the
nonexistent path parameter with Sphinx :param: entries for inpath, outpath,
version, force, and compression, matching the method signature names. Keep type
information in the existing annotations rather than adding types to the
docstring, and use Sphinx-style documentation consistently.
In `@src/aiida/tools/dbimporters/plugins/icsd.py`:
- Line 114: Parameterize every caller-controlled value in both _str_exact_clause
and _composition_clause in src/aiida/tools/dbimporters/plugins/icsd.py at lines
114-114 and 141-141, replacing direct SQL interpolation with bound parameters or
trusted driver escaping. Preserve the existing exact-match predicate and the
intended regular-expression pattern in _composition_clause; both sites require
direct changes.
- Line 141: Update the REGEXP expression in the values-join logic to use a
double-quoted raw f-string, removing the unnecessary quote escapes so the
generated SQL contains no stray backslashes.
In `@src/aiida/tools/pytest_fixtures/orm.py`:
- Line 62: Update the return annotations of aiida_computer and the other two
factory fixtures in orm.py to reflect their actual callable signatures, or use
Callable[..., Computer] when exact signatures cannot be expressed. Apply the
same broader typing consistently to all three returned factories.
In `@tests/tools/dumping/integration_tests.py`:
- Around line 247-250: Update the get_expected_profile_dump_tree docstring by
replacing its Google-style Args and Returns sections with Sphinx :param field
entries and a :return: field. Keep all type information in the function
annotations and remove duplicated type declarations from the docstring.
---
Outside diff comments:
In `@src/aiida/transports/transport.py`:
- Around line 321-331: Update _gotocomputer_string to shell-escape remotedir
using the existing escaping helper or shlex.quote before interpolating it into
the generated command. Reuse the escaped value for every remotedir occurrence,
including the directory test and error message, while preserving the existing
command flow.
---
Nitpick comments:
In `@src/aiida/engine/processes/ports.py`:
- Line 261: Update the serialize method parameter annotation to accept
Mapping[str, Any] | None instead of dict[str, Any] | None, while preserving its
dict[str, Any] | None return annotation and implementation behavior.
In `@src/aiida/orm/implementation/storage_backend.py`:
- Line 16: Replace the deprecated typing.ContextManager import with
contextlib.AbstractContextManager, then update the corresponding annotation in
the implementation using ContextManager to reference AbstractContextManager
while preserving its existing type parameters and behavior.
In `@src/aiida/orm/nodes/data/remote/stash/folder.py`:
- Around line 80-88: Update the source_list property getter and setter in the
source_list definition to use list[str] | tuple[str, ...] instead of list |
tuple, preserving the required string-path element type for static consumers
while leaving the existing behavior unchanged.
In `@src/aiida/orm/nodes/process/process.py`:
- Line 179: Update the _hash_ignored_attributes property to extend the
superclass tuple using iterable unpacking instead of tuple concatenation,
preserving the existing metadata_inputs value and tuple result.
In `@src/aiida/tools/_dumping/tracking.py`:
- Around line 14-18: Update the return annotation of iter_by_type() to use
collections.abc.Iterator for its yielded values, and replace the Generator
import if it is no longer used elsewhere in the module. Verify the change with
the repository’s mypy target.
In `@src/aiida/tools/data/array/kpoints/legacy.py`:
- Line 1088: Update the permutation construction near the affected k-point
conversion logic to avoid list concatenation flagged by Ruff RUF005 and replace
set-based iteration with a deterministic ordering for the two remaining axes.
Preserve in_line_index as the first element and append the remaining axis
indices via ordered iterable unpacking.
In `@src/aiida/transports/plugins/async_backend.py`:
- Line 98: Align the timeout annotations across the async backend contract by
changing the abstract backend run method and the `_AsyncSSH` implementation to
accept `float | None`, matching `AsyncSshTransport.exec_command_wait_async` and
`_OpenSSH`. Update all referenced `run` signatures while preserving their
existing timeout behavior.
In `@tests/utils/archives.py`:
- Around line 81-88: Complete the Sphinx-style docstring for read_json_files by
adding a :return: description for the list[dict] result and a :raises
ValueError: entry covering unsupported archive formats, while preserving the
existing parameter documentation and type annotations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2fdd849c-1c7a-46b1-9d89-cd781fc7702a
📒 Files selected for processing (210)
.molecule/default/files/polish/lib/workchain.pydocs/source/conf.pydocs/source/topics/processes/include/snippets/functions/typing_none.pysrc/aiida/brokers/broker.pysrc/aiida/brokers/rabbitmq/broker.pysrc/aiida/brokers/rabbitmq/client.pysrc/aiida/brokers/zeromq/broker.pysrc/aiida/brokers/zeromq/communicator.pysrc/aiida/brokers/zeromq/server.pysrc/aiida/calculations/diff_tutorial/calculations.pysrc/aiida/calculations/importers/arithmetic/add.pysrc/aiida/calculations/stash.pysrc/aiida/cmdline/commands/cmd_bug_report.pysrc/aiida/cmdline/commands/cmd_code.pysrc/aiida/cmdline/commands/cmd_config.pysrc/aiida/cmdline/commands/cmd_node.pysrc/aiida/cmdline/commands/cmd_process.pysrc/aiida/cmdline/commands/cmd_rabbitmq.pysrc/aiida/cmdline/params/types/path.pysrc/aiida/cmdline/utils/common.pysrc/aiida/cmdline/utils/echo.pysrc/aiida/common/hashing.pysrc/aiida/common/lang.pysrc/aiida/common/progress_reporter.pysrc/aiida/common/timezone.pysrc/aiida/common/utils.pysrc/aiida/engine/daemon/client.pysrc/aiida/engine/daemon/execmanager.pysrc/aiida/engine/daemon/worker.pysrc/aiida/engine/persistence.pysrc/aiida/engine/processes/builder.pysrc/aiida/engine/processes/calcjobs/calcjob.pysrc/aiida/engine/processes/calcjobs/importer.pysrc/aiida/engine/processes/calcjobs/manager.pysrc/aiida/engine/processes/calcjobs/tasks.pysrc/aiida/engine/processes/control.pysrc/aiida/engine/processes/exit_code.pysrc/aiida/engine/processes/functions.pysrc/aiida/engine/processes/futures.pysrc/aiida/engine/processes/ports.pysrc/aiida/engine/processes/process.pysrc/aiida/engine/processes/process_spec.pysrc/aiida/engine/processes/workchains/awaitable.pysrc/aiida/engine/processes/workchains/context.pysrc/aiida/engine/processes/workchains/restart.pysrc/aiida/engine/processes/workchains/utils.pysrc/aiida/engine/processes/workchains/workchain.pysrc/aiida/engine/runners.pysrc/aiida/engine/transports.pysrc/aiida/engine/utils.pysrc/aiida/manage/configuration/__init__.pysrc/aiida/manage/configuration/config.pysrc/aiida/manage/configuration/migrations/migrations.pysrc/aiida/manage/configuration/options.pysrc/aiida/manage/configuration/profile.pysrc/aiida/manage/manager.pysrc/aiida/manage/profile_access.pysrc/aiida/manage/tests/pytest_fixtures.pysrc/aiida/orm/authinfos.pysrc/aiida/orm/comments.pysrc/aiida/orm/computers.pysrc/aiida/orm/entities.pysrc/aiida/orm/extras.pysrc/aiida/orm/fields.pysrc/aiida/orm/groups.pysrc/aiida/orm/implementation/authinfos.pysrc/aiida/orm/implementation/comments.pysrc/aiida/orm/implementation/computers.pysrc/aiida/orm/implementation/entities.pysrc/aiida/orm/implementation/groups.pysrc/aiida/orm/implementation/logs.pysrc/aiida/orm/implementation/nodes.pysrc/aiida/orm/implementation/querybuilder.pysrc/aiida/orm/implementation/storage_backend.pysrc/aiida/orm/logs.pysrc/aiida/orm/nodes/attributes.pysrc/aiida/orm/nodes/caching.pysrc/aiida/orm/nodes/comments.pysrc/aiida/orm/nodes/data/array/array.pysrc/aiida/orm/nodes/data/array/bands.pysrc/aiida/orm/nodes/data/array/trajectory.pysrc/aiida/orm/nodes/data/array/xy.pysrc/aiida/orm/nodes/data/cif.pysrc/aiida/orm/nodes/data/code/abstract.pysrc/aiida/orm/nodes/data/data.pysrc/aiida/orm/nodes/data/dict.pysrc/aiida/orm/nodes/data/remote/base.pysrc/aiida/orm/nodes/data/remote/stash/compress.pysrc/aiida/orm/nodes/data/remote/stash/custom.pysrc/aiida/orm/nodes/data/remote/stash/folder.pysrc/aiida/orm/nodes/data/structure.pysrc/aiida/orm/nodes/links.pysrc/aiida/orm/nodes/node.pysrc/aiida/orm/nodes/process/calculation/calcjob.pysrc/aiida/orm/nodes/process/process.pysrc/aiida/orm/nodes/process/workflow/workchain.pysrc/aiida/orm/nodes/repository.pysrc/aiida/orm/querybuilder.pysrc/aiida/orm/users.pysrc/aiida/orm/utils/builders/code.pysrc/aiida/orm/utils/calcjob.pysrc/aiida/orm/utils/links.pysrc/aiida/orm/utils/managers.pysrc/aiida/orm/utils/mixins.pysrc/aiida/orm/utils/serialize.pysrc/aiida/parsers/parser.pysrc/aiida/parsers/plugins/templatereplacer/parser.pysrc/aiida/plugins/entry_point.pysrc/aiida/plugins/factories.pysrc/aiida/repository/backend/abstract.pysrc/aiida/repository/common.pysrc/aiida/repository/repository.pysrc/aiida/schedulers/plugins/pbsbaseclasses.pysrc/aiida/storage/psql_dos/backend.pysrc/aiida/storage/psql_dos/migrations/utils/dblog_update.pysrc/aiida/storage/psql_dos/migrations/utils/provenance_redesign.pysrc/aiida/storage/psql_dos/migrations/utils/utils.pysrc/aiida/storage/psql_dos/migrator.pysrc/aiida/storage/psql_dos/orm/entities.pysrc/aiida/storage/psql_dos/orm/extras_mixin.pysrc/aiida/storage/psql_dos/orm/nodes.pysrc/aiida/storage/psql_dos/orm/querybuilder/joiner.pysrc/aiida/storage/psql_dos/orm/querybuilder/main.pysrc/aiida/storage/psql_dos/orm/utils.pysrc/aiida/storage/sqlite_dos/backend.pysrc/aiida/storage/sqlite_temp/backend.pysrc/aiida/storage/sqlite_zip/backend.pysrc/aiida/storage/sqlite_zip/migrations/legacy/__init__.pysrc/aiida/storage/sqlite_zip/migrations/legacy/v05_to_v06.pysrc/aiida/storage/sqlite_zip/migrations/legacy_to_main.pysrc/aiida/storage/sqlite_zip/migrations/utils.pysrc/aiida/storage/sqlite_zip/migrator.pysrc/aiida/storage/sqlite_zip/models.pysrc/aiida/storage/sqlite_zip/orm.pysrc/aiida/storage/sqlite_zip/utils.pysrc/aiida/storage/utils.pysrc/aiida/tools/_dumping/config.pysrc/aiida/tools/_dumping/detect.pysrc/aiida/tools/_dumping/engine.pysrc/aiida/tools/_dumping/executors/collection.pysrc/aiida/tools/_dumping/executors/deletion.pysrc/aiida/tools/_dumping/executors/process.pysrc/aiida/tools/_dumping/mapping.pysrc/aiida/tools/_dumping/tracking.pysrc/aiida/tools/_dumping/utils.pysrc/aiida/tools/archive/abstract.pysrc/aiida/tools/archive/common.pysrc/aiida/tools/archive/create.pysrc/aiida/tools/archive/implementations/sqlite_zip/main.pysrc/aiida/tools/archive/implementations/sqlite_zip/reader.pysrc/aiida/tools/archive/implementations/sqlite_zip/writer.pysrc/aiida/tools/archive/imports.pysrc/aiida/tools/data/array/kpoints/legacy.pysrc/aiida/tools/dbimporters/baseclasses.pysrc/aiida/tools/dbimporters/plugins/icsd.pysrc/aiida/tools/dbimporters/plugins/materialsproject.pysrc/aiida/tools/dbimporters/plugins/nninc.pysrc/aiida/tools/graph/age_entities.pysrc/aiida/tools/graph/deletions.pysrc/aiida/tools/graph/graph_traversers.pysrc/aiida/tools/groups/paths.pysrc/aiida/tools/pytest_fixtures/configuration.pysrc/aiida/tools/pytest_fixtures/daemon.pysrc/aiida/tools/pytest_fixtures/entry_points.pysrc/aiida/tools/pytest_fixtures/orm.pysrc/aiida/tools/pytest_fixtures/storage.pysrc/aiida/tools/query/mapping.pysrc/aiida/tools/visualization/graph.pysrc/aiida/transports/plugins/async_backend.pysrc/aiida/transports/plugins/local.pysrc/aiida/transports/plugins/ssh.pysrc/aiida/transports/plugins/ssh_async.pysrc/aiida/transports/transport.pytests/calculations/test_transfer.pytests/cmdline/commands/test_bug_report.pytests/cmdline/commands/test_data.pytests/cmdline/commands/test_node.pytests/cmdline/commands/test_storage.pytests/cmdline/params/types/test_path.pytests/common/test_extendeddicts.pytests/common/test_hashing.pytests/conftest.pytests/engine/processes/calcjobs/test_calc_job.pytests/engine/test_process.pytests/manage/configuration/test_config.pytests/orm/nodes/data/test_upf.pytests/orm/nodes/test_node.pytests/orm/test_autogroups.pytests/orm/test_groups.pytests/orm/test_querybuilder.pytests/orm/utils/test_managers.pytests/repository/backend/test_abstract.pytests/repository/conftest.pytests/restapi/conftest.pytests/storage/psql_dos/migrations/django_branch/test_0047_migrate_repository.pytests/storage/psql_dos/migrations/sqlalchemy_branch/test_11_v2_repository.pytests/storage/psql_dos/test_schema.pytests/test_dataclasses.pytests/test_dbimporters.pytests/test_nodes.pytests/tools/archive/orm/test_computers.pytests/tools/archive/orm/test_links.pytests/tools/dumping/integration_tests.pytests/tools/dumping/utils.pytests/tools/graph/test_age.pytests/tools/graph/test_graph_traversers.pytests/tools/visualization/test_graph.pytests/transports/test_all_plugins.pytests/utils/archives.pyutils/autogenerate_all_imports.py
💤 Files with no reviewable changes (1)
- tests/restapi/conftest.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/aiida/engine/processes/functions.py (1)
249-257: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the duplicate
dictandlistkeys.The mapping currently contains each key twice. Python silently overwrites the first entry, and Ruff reports F602.
Proposed fix
bool: Bool, dict: Dict, - dict: Dict, float: Float, int: Int, list: List, - list: List, str: Str,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/engine/processes/functions.py` around lines 249 - 257, Remove the duplicate dict: Dict and list: List entries from valid_type_map in get_type_from_annotation, leaving one mapping for each key so the mapping remains unchanged while eliminating Ruff F602.Source: Linters/SAST tools
.github/system_tests/test_daemon.py (1)
163-186: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNew f-strings exceed Ruff’s 120-char limit (
.github/system_tests/test_daemon.py:163-186, also195-201and218-227)
Several of these diagnostics now run past the configuredline-length = 120; split them back across multiple literals to avoid lint failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/system_tests/test_daemon.py around lines 163 - 186, The diagnostic f-strings in the test daemon exceed Ruff’s 120-character limit. Reformat the messages in the shown exception, calculation-status, unexpected-value, and corresponding sections around lines 195–201 and 218–227 by splitting them across adjacent string literals while preserving their output and values.
🧹 Nitpick comments (8)
src/aiida/engine/launch.py (1)
30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse PEP 604 syntax for the complete type aliases.
These aliases still use
t.Union, so Ruff’sUPrules may continue to flag them. Modernize both aliases consistently:Proposed fix
-TYPE_RUN_PROCESS = t.Union[Process, type[Process], ProcessBuilder] +TYPE_RUN_PROCESS = Process | type[Process] | ProcessBuilder ... -TYPE_SUBMIT_PROCESS = t.Union[Process, type[Process], ProcessBuilder] +TYPE_SUBMIT_PROCESS = Process | type[Process] | ProcessBuilder🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/engine/launch.py` around lines 30 - 32, Update the TYPE_RUN_PROCESS and TYPE_SUBMIT_PROCESS aliases to use PEP 604 union syntax with the | operator instead of t.Union, preserving all existing union members and keeping both aliases consistent.src/aiida/common/folders.py (1)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssign the exception message before raising.
As per coding guidelines: Assign exception messages to a variable before raising, e.g.
msg = f'...'; raise ValueError(msg).Proposed fix
+ msg = f'folder_limit. abspath={abspath}, folder_limit={folder_limit}.' - f'folder_limit. abspath={abspath}, folder_limit={folder_limit}.' + raise ValueError(msg)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/common/folders.py` at line 65, In the folder validation logic surrounding the shown f-string, assign the formatted exception text to a local variable such as msg before raising the exception, then pass that variable to the raise statement. Preserve the existing message content and exception type.Source: Coding guidelines
src/aiida/tools/graph/age_entities.py (1)
211-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssign exception messages to a variable before raising, across all mechanically converted
.format()→ f-string sites.As per coding guidelines,
**/*.py: "Assign exception messages to a variable before raising, e.g.msg = f'...'; raise TypeError(msg)." Every site below still constructs the message inline inside theraise(or, for ssh.py's second_exec_cpblock, two exception types built inline).
src/aiida/tools/graph/age_entities.py#L211-L215: assign theValueErrormessage in_check_input_for_settomsgbefore raising.src/aiida/tools/graph/age_entities.py#L376-L379: assign theValueErrormessage inget_check_set_entity_settomsgbefore raising.src/aiida/transports/plugins/ssh.py#L729-L743: assign eachOSErrormessage inmkdirtomsgbefore raising.src/aiida/transports/plugins/ssh.py#L1289-L1297: assign theFileNotFoundError/OSErrormessages in_exec_cptomsgbefore raising.src/aiida/calculations/templatereplacer.py#L153-L157: assign theInputValidationErrormessage tomsgbefore raising.src/aiida/schedulers/plugins/lsf.py#L410-L417: assign theValueErrormessage for invalidmax_wallclock_secondstomsgbefore raising.src/aiida/schedulers/plugins/pbspro.py#L70-L78: assign theValueErrormessage for invalidmax_wallclock_secondstomsgbefore raising.src/aiida/schedulers/plugins/torque.py#L64-L72: assign theValueErrormessage for invalidmax_wallclock_secondstomsgbefore raising.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/tools/graph/age_entities.py` around lines 211 - 215, Assign each exception message to a local variable named msg before raising, preserving the existing message text and exception types. Apply this in _check_input_for_set and get_check_set_entity_set in src/aiida/tools/graph/age_entities.py (lines 211-215 and 376-379), mkdir and _exec_cp in src/aiida/transports/plugins/ssh.py (lines 729-743 and 1289-1297), and the relevant validation raises in src/aiida/calculations/templatereplacer.py (lines 153-157), src/aiida/schedulers/plugins/lsf.py (lines 410-417), src/aiida/schedulers/plugins/pbspro.py (lines 70-78), and src/aiida/schedulers/plugins/torque.py (lines 64-72); for _exec_cp, assign each distinct message before its corresponding FileNotFoundError or OSError raise.Source: Coding guidelines
src/aiida/orm/nodes/data/upf.py (2)
264-268: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssign the exception message to a variable before raising.
As per coding guidelines, "Assign exception messages to a variable before raising, e.g.
msg = f'...'; raise TypeError(msg)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/nodes/data/upf.py` around lines 264 - 268, Update the check_filename validation block to assign the formatted filename validation message to a local variable before raising ParsingError, then pass that variable to the exception without changing the message or validation behavior.Source: Coding guidelines
133-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssign the exception message to a variable before raising.
As per coding guidelines, "Assign exception messages to a variable before raising, e.g.
msg = f'...'; raise TypeError(msg)."♻️ Proposed fix
if group.user.email != default_user.email: - raise UniquenessError( - f'There is already a UpfFamily group with label {group_label}' - f', but it belongs to user {group.user.email}, therefore you ' - 'cannot modify it' - ) + msg = ( + f'There is already a UpfFamily group with label {group_label}' + f', but it belongs to user {group.user.email}, therefore you ' + 'cannot modify it' + ) + raise UniquenessError(msg)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/nodes/data/upf.py` around lines 133 - 137, In the UpfFamily uniqueness check, update the raise block to first assign the formatted exception message to a local variable, then pass that variable to UniquenessError. Preserve the existing message content and exception type.Source: Coding guidelines
src/aiida/orm/nodes/data/array/kpoints.py (2)
438-441: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssign the exception message to a variable before raising.
As per coding guidelines, "Assign exception messages to a variable before raising, e.g.
msg = f'...'; raise TypeError(msg)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/nodes/data/array/kpoints.py` around lines 438 - 441, In the error branch of the kpoints validation logic, assign the formatted ValueError message to a local variable before raising it. Update the existing raise in the surrounding dimension-checking logic to raise using that variable, without changing the message or exception type.Source: Coding guidelines
283-286: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssign the exception message to a variable before raising.
As per coding guidelines, "Assign exception messages to a variable before raising, e.g.
msg = f'...'; raise TypeError(msg)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/nodes/data/array/kpoints.py` around lines 283 - 286, Update the validation branch in the KpointsData structure handling to assign the formatted exception message to a local variable before raising ValueError, then raise using that variable while preserving the existing message content.Source: Coding guidelines
src/aiida/orm/nodes/process/workflow/workflow.py (1)
41-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssign the exception message to a variable before raising.
As per coding guidelines, "Assign exception messages to a variable before raising, e.g.
msg = f'...'; raise TypeError(msg)."♻️ Proposed fix
if link_type is LinkType.RETURN and not target.is_stored: - raise ValueError( - f'Workflow<{self._node.process_label}> tried returning an unstored `Data` node. This likely means new `Data` is being created ' - 'inside the workflow. In order to preserve data provenance, use a `calcfunction` to create this node ' - 'and return its output from the workflow' - ) + msg = ( + f'Workflow<{self._node.process_label}> tried returning an unstored `Data` node. This likely means new `Data` is being created ' + 'inside the workflow. In order to preserve data provenance, use a `calcfunction` to create this node ' + 'and return its output from the workflow' + ) + raise ValueError(msg)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/nodes/process/workflow/workflow.py` around lines 41 - 46, In the RETURN-link validation block, assign the existing unstored-Data error message to a local variable before raising the exception, then raise ValueError using that variable. Preserve the current message text and condition in the workflow link-handling logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aiida/orm/nodes/data/array/kpoints.py`:
- Around line 454-457: Update the ValueError construction in the kpoints
validation block to correct “needmore” to “need more”, assign the formatted
message to a local variable first, and raise ValueError using that variable.
In `@src/aiida/tools/graph/age_rules.py`:
- Around line 116-119: Assign each exception message to a local variable named
msg before raising: the projection-validation message in age_rules.py,
wallclock-validation messages in sge.py and slurm.py, the invalid-node-id
message in translator/base.py, and the unsupported-format message in
translator/nodes/data/__init__.py. Raise the corresponding ValueError or
RestInputValidationError with msg while preserving the existing message content.
---
Outside diff comments:
In @.github/system_tests/test_daemon.py:
- Around line 163-186: The diagnostic f-strings in the test daemon exceed Ruff’s
120-character limit. Reformat the messages in the shown exception,
calculation-status, unexpected-value, and corresponding sections around lines
195–201 and 218–227 by splitting them across adjacent string literals while
preserving their output and values.
In `@src/aiida/engine/processes/functions.py`:
- Around line 249-257: Remove the duplicate dict: Dict and list: List entries
from valid_type_map in get_type_from_annotation, leaving one mapping for each
key so the mapping remains unchanged while eliminating Ruff F602.
---
Nitpick comments:
In `@src/aiida/common/folders.py`:
- Line 65: In the folder validation logic surrounding the shown f-string, assign
the formatted exception text to a local variable such as msg before raising the
exception, then pass that variable to the raise statement. Preserve the existing
message content and exception type.
In `@src/aiida/engine/launch.py`:
- Around line 30-32: Update the TYPE_RUN_PROCESS and TYPE_SUBMIT_PROCESS aliases
to use PEP 604 union syntax with the | operator instead of t.Union, preserving
all existing union members and keeping both aliases consistent.
In `@src/aiida/orm/nodes/data/array/kpoints.py`:
- Around line 438-441: In the error branch of the kpoints validation logic,
assign the formatted ValueError message to a local variable before raising it.
Update the existing raise in the surrounding dimension-checking logic to raise
using that variable, without changing the message or exception type.
- Around line 283-286: Update the validation branch in the KpointsData structure
handling to assign the formatted exception message to a local variable before
raising ValueError, then raise using that variable while preserving the existing
message content.
In `@src/aiida/orm/nodes/data/upf.py`:
- Around line 264-268: Update the check_filename validation block to assign the
formatted filename validation message to a local variable before raising
ParsingError, then pass that variable to the exception without changing the
message or validation behavior.
- Around line 133-137: In the UpfFamily uniqueness check, update the raise block
to first assign the formatted exception message to a local variable, then pass
that variable to UniquenessError. Preserve the existing message content and
exception type.
In `@src/aiida/orm/nodes/process/workflow/workflow.py`:
- Around line 41-46: In the RETURN-link validation block, assign the existing
unstored-Data error message to a local variable before raising the exception,
then raise ValueError using that variable. Preserve the current message text and
condition in the workflow link-handling logic.
In `@src/aiida/tools/graph/age_entities.py`:
- Around line 211-215: Assign each exception message to a local variable named
msg before raising, preserving the existing message text and exception types.
Apply this in _check_input_for_set and get_check_set_entity_set in
src/aiida/tools/graph/age_entities.py (lines 211-215 and 376-379), mkdir and
_exec_cp in src/aiida/transports/plugins/ssh.py (lines 729-743 and 1289-1297),
and the relevant validation raises in src/aiida/calculations/templatereplacer.py
(lines 153-157), src/aiida/schedulers/plugins/lsf.py (lines 410-417),
src/aiida/schedulers/plugins/pbspro.py (lines 70-78), and
src/aiida/schedulers/plugins/torque.py (lines 64-72); for _exec_cp, assign each
distinct message before its corresponding FileNotFoundError or OSError raise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fa250c66-f6dc-4e42-89d0-ea827806de52
📒 Files selected for processing (75)
.github/system_tests/test_daemon.pydocs/source/topics/processes/include/snippets/functions/typing_union.pypyproject.tomlsrc/aiida/calculations/templatereplacer.pysrc/aiida/cmdline/commands/cmd_calcjob.pysrc/aiida/cmdline/commands/cmd_storage.pysrc/aiida/cmdline/groups/verdi.pysrc/aiida/cmdline/params/options/callable.pysrc/aiida/cmdline/params/options/main.pysrc/aiida/cmdline/params/types/code.pysrc/aiida/cmdline/params/types/identifier.pysrc/aiida/cmdline/params/types/plugin.pysrc/aiida/common/folders.pysrc/aiida/common/hashing.pysrc/aiida/common/utils.pysrc/aiida/engine/launch.pysrc/aiida/engine/processes/calcjobs/calcjob.pysrc/aiida/engine/processes/control.pysrc/aiida/engine/processes/functions.pysrc/aiida/engine/processes/ports.pysrc/aiida/engine/processes/process.pysrc/aiida/manage/tests/pytest_fixtures.pysrc/aiida/orm/fields.pysrc/aiida/orm/nodes/comments.pysrc/aiida/orm/nodes/data/array/kpoints.pysrc/aiida/orm/nodes/data/array/trajectory.pysrc/aiida/orm/nodes/data/code/abstract.pysrc/aiida/orm/nodes/data/code/legacy.pysrc/aiida/orm/nodes/data/data.pysrc/aiida/orm/nodes/data/dict.pysrc/aiida/orm/nodes/data/enum.pysrc/aiida/orm/nodes/data/remote/base.pysrc/aiida/orm/nodes/data/structure.pysrc/aiida/orm/nodes/data/upf.pysrc/aiida/orm/nodes/links.pysrc/aiida/orm/nodes/process/workflow/workflow.pysrc/aiida/orm/pydantic.pysrc/aiida/orm/querybuilder.pysrc/aiida/orm/utils/links.pysrc/aiida/parsers/plugins/templatereplacer/parser.pysrc/aiida/repository/backend/abstract.pysrc/aiida/repository/backend/disk_object_store.pysrc/aiida/restapi/translator/base.pysrc/aiida/restapi/translator/nodes/data/__init__.pysrc/aiida/schedulers/plugins/lsf.pysrc/aiida/schedulers/plugins/pbspro.pysrc/aiida/schedulers/plugins/sge.pysrc/aiida/schedulers/plugins/slurm.pysrc/aiida/schedulers/plugins/torque.pysrc/aiida/schedulers/scheduler.pysrc/aiida/storage/psql_dos/migrations/utils/dblog_update.pysrc/aiida/storage/psql_dos/migrations/utils/integrity.pysrc/aiida/storage/psql_dos/orm/querybuilder/joiner.pysrc/aiida/tools/graph/age_entities.pysrc/aiida/tools/graph/age_rules.pysrc/aiida/tools/graph/graph_traversers.pysrc/aiida/tools/visualization/graph.pysrc/aiida/transports/cli.pysrc/aiida/transports/plugins/async_backend.pysrc/aiida/transports/plugins/ssh.pysrc/aiida/transports/plugins/ssh_async.pysrc/aiida/transports/transport.pytests/cmdline/commands/test_archive_import.pytests/cmdline/groups/test_dynamic.pytests/conftest.pytests/engine/test_process_function.pytests/orm/implementation/test_comments.pytests/orm/implementation/test_logs.pytests/orm/nodes/data/test_data.pytests/orm/test_entities.pytests/repository/test_repository.pytests/restapi/test_identifiers.pytests/tools/archive/orm/test_groups.pytests/tools/archive/orm/test_links.pytests/transports/test_all_plugins.py
🚧 Files skipped from review as they are similar to previous changes (22)
- src/aiida/tools/visualization/graph.py
- src/aiida/orm/nodes/comments.py
- src/aiida/orm/nodes/data/dict.py
- src/aiida/parsers/plugins/templatereplacer/parser.py
- src/aiida/orm/nodes/data/remote/base.py
- src/aiida/common/utils.py
- src/aiida/storage/psql_dos/orm/querybuilder/joiner.py
- src/aiida/orm/nodes/data/data.py
- src/aiida/storage/psql_dos/migrations/utils/dblog_update.py
- src/aiida/engine/processes/ports.py
- src/aiida/engine/processes/control.py
- src/aiida/transports/plugins/ssh_async.py
- src/aiida/transports/plugins/async_backend.py
- src/aiida/common/hashing.py
- src/aiida/engine/processes/calcjobs/calcjob.py
- src/aiida/orm/nodes/links.py
- src/aiida/tools/graph/graph_traversers.py
- src/aiida/orm/utils/links.py
- src/aiida/repository/backend/abstract.py
- src/aiida/transports/transport.py
- src/aiida/engine/processes/process.py
- src/aiida/orm/querybuilder.py
|
@coderabbitai resolve |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/engine/test_process_function.py (1)
706-737: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale docstring/comment after removing the version-conditional branch.
The docstring (lines 707-715) and inline comment (lines 727-728) still describe the old Python-version-dependent behavior ("type inference using this syntax only works on Python 3.10 and up", "valid type will be the default
(orm.Data,)"), but theexpectedtuple right below now unconditionally asserts the fully-inferred types ((orm.Str, orm.Int),(orm.Dict, type(None))). The comment directly contradicts the code it precedes.Additionally, the
caplogfixture parameter appears to now be unused in this test since the branch that asserted an "unsupported operand type" log message on older Python versions was removed.📝 Suggested cleanup
-def test_type_hinting_spec_inference_pep_604(caplog): - """Test the parsing of type hinting that uses union typing of PEP 604 which is only available to Python 3.10 and up. - - Even though adding ``from __future__ import annotations`` should backport this functionality to Python 3.9 and older - the ``get_annotations`` method (which was also added to the ``inspect`` package in Python 3.10) as provided by the - ``get-annotations`` backport package fails for this new syntax when called with ``eval_str=True``. Therefore type - inference using this syntax only works on Python 3.10 and up. - - See https://peps.python.org/pep-0604 - """ +def test_type_hinting_spec_inference_pep_604(): + """Test the parsing of type hinting that uses PEP 604 union syntax. + + See https://peps.python.org/pep-0604 + """ `@calcfunction` # type: ignore[misc] def function( a: str | int, b: orm.Str | orm.Int, c: dict | None = None, ): pass input_namespace = function.spec().inputs - # Since the PEP 604 union syntax is only available starting from Python 3.10 the type inference will not be - # available for older versions, and so the valid type will be the default ``(orm.Data,)``. expected = ( ('a', (orm.Str, orm.Int)), ('b', (orm.Str, orm.Int)), ('c', (orm.Dict, type(None))), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/engine/test_process_function.py` around lines 706 - 737, The test’s documentation and signature still describe removed Python-version-dependent behavior. Update test_type_hinting_spec_inference_pep_604 to document unconditional PEP 604 type inference matching the expected valid_type assertions, remove the obsolete version-conditional inline comment, and drop the unused caplog parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/engine/test_process_function.py`:
- Around line 706-737: The test’s documentation and signature still describe
removed Python-version-dependent behavior. Update
test_type_hinting_spec_inference_pep_604 to document unconditional PEP 604 type
inference matching the expected valid_type assertions, remove the obsolete
version-conditional inline comment, and drop the unused caplog parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eb87b4bb-cb16-433c-865a-107dcf3bf647
📒 Files selected for processing (50)
.github/system_tests/test_daemon.pydocs/source/topics/processes/include/snippets/functions/typing_union.pysrc/aiida/calculations/templatereplacer.pysrc/aiida/cmdline/commands/cmd_calcjob.pysrc/aiida/cmdline/commands/cmd_storage.pysrc/aiida/cmdline/params/types/code.pysrc/aiida/cmdline/params/types/identifier.pysrc/aiida/cmdline/params/types/path.pysrc/aiida/common/hashing.pysrc/aiida/common/typing.pysrc/aiida/common/utils.pysrc/aiida/engine/launch.pysrc/aiida/engine/processes/calcjobs/calcjob.pysrc/aiida/engine/processes/functions.pysrc/aiida/engine/processes/ports.pysrc/aiida/engine/runners.pysrc/aiida/orm/implementation/storage_backend.pysrc/aiida/orm/nodes/data/array/bands.pysrc/aiida/orm/nodes/data/array/kpoints.pysrc/aiida/orm/nodes/data/code/legacy.pysrc/aiida/orm/nodes/data/data.pysrc/aiida/orm/nodes/data/remote/base.pysrc/aiida/orm/nodes/data/structure.pysrc/aiida/orm/nodes/process/workflow/workflow.pysrc/aiida/orm/querybuilder.pysrc/aiida/orm/utils/links.pysrc/aiida/parsers/plugins/templatereplacer/parser.pysrc/aiida/restapi/translator/base.pysrc/aiida/schedulers/plugins/lsf.pysrc/aiida/schedulers/plugins/pbspro.pysrc/aiida/schedulers/plugins/sge.pysrc/aiida/schedulers/plugins/slurm.pysrc/aiida/schedulers/plugins/torque.pysrc/aiida/storage/psql_dos/migrations/utils/dblog_update.pysrc/aiida/storage/psql_dos/migrations/utils/integrity.pysrc/aiida/storage/psql_dos/orm/querybuilder/joiner.pysrc/aiida/storage/sqlite_zip/migrations/legacy_to_main.pysrc/aiida/tools/_dumping/utils.pysrc/aiida/tools/graph/age_rules.pysrc/aiida/tools/ipython/ipython_magics.pysrc/aiida/tools/visualization/graph.pysrc/aiida/transports/plugins/async_backend.pysrc/aiida/transports/plugins/ssh.pysrc/aiida/transports/plugins/ssh_async.pysrc/aiida/transports/transport.pytests/cmdline/commands/test_archive_import.pytests/engine/test_process_function.pytests/restapi/conftest.pytests/tools/archive/orm/test_groups.pytests/tools/archive/orm/test_links.py
💤 Files with no reviewable changes (2)
- docs/source/topics/processes/include/snippets/functions/typing_union.py
- src/aiida/engine/processes/functions.py
🚧 Files skipped from review as they are similar to previous changes (41)
- src/aiida/schedulers/plugins/torque.py
- src/aiida/schedulers/plugins/pbspro.py
- src/aiida/cmdline/params/types/identifier.py
- src/aiida/calculations/templatereplacer.py
- src/aiida/orm/nodes/process/workflow/workflow.py
- src/aiida/tools/graph/age_rules.py
- src/aiida/engine/launch.py
- src/aiida/cmdline/commands/cmd_storage.py
- src/aiida/tools/visualization/graph.py
- src/aiida/cmdline/params/types/code.py
- src/aiida/schedulers/plugins/sge.py
- tests/tools/archive/orm/test_groups.py
- src/aiida/restapi/translator/base.py
- src/aiida/schedulers/plugins/lsf.py
- src/aiida/storage/psql_dos/migrations/utils/integrity.py
- src/aiida/schedulers/plugins/slurm.py
- src/aiida/common/utils.py
- src/aiida/cmdline/params/types/path.py
- tests/tools/archive/orm/test_links.py
- src/aiida/orm/nodes/data/remote/base.py
- src/aiida/orm/nodes/data/code/legacy.py
- src/aiida/transports/plugins/async_backend.py
- src/aiida/cmdline/commands/cmd_calcjob.py
- src/aiida/orm/nodes/data/array/kpoints.py
- src/aiida/engine/processes/ports.py
- src/aiida/orm/nodes/data/data.py
- tests/cmdline/commands/test_archive_import.py
- src/aiida/common/hashing.py
- src/aiida/storage/psql_dos/migrations/utils/dblog_update.py
- src/aiida/transports/plugins/ssh_async.py
- src/aiida/orm/utils/links.py
- src/aiida/engine/runners.py
- src/aiida/orm/implementation/storage_backend.py
- src/aiida/orm/nodes/data/structure.py
- src/aiida/engine/processes/calcjobs/calcjob.py
- .github/system_tests/test_daemon.py
- src/aiida/storage/psql_dos/orm/querybuilder/joiner.py
- src/aiida/transports/plugins/ssh.py
- src/aiida/tools/_dumping/utils.py
- src/aiida/transports/transport.py
- src/aiida/orm/querybuilder.py
✅ Action performedComments resolved. Approval is disabled; enable |
4fb38e7 to
de39b86
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
src/aiida/engine/processes/ports.py (1)
262-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccept
Mappinginputs in the annotation.The implementation explicitly accepts any
Mapping, but the signature restricts callers todict. UseMapping[str, Any] | Noneto match the runtime contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/engine/processes/ports.py` at line 262, Update the serialize method annotation to accept Mapping[str, Any] | None instead of dict[str, Any] | None, matching its existing runtime support for mapping inputs while leaving the implementation unchanged.src/aiida/engine/daemon/client.py (1)
568-568: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
get_clientasIterator[CircusClient]. The@contextlib.contextmanagerfunction yieldsCircusClient, so the signature should reflect an iterator; addIteratorfromcollections.abcif needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/engine/daemon/client.py` at line 568, Update the get_client method annotation to return Iterator[CircusClient], matching its contextmanager yield behavior, and import Iterator from collections.abc if it is not already available.src/aiida/schedulers/plugins/slurm.py (1)
365-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssign the exception message before raising.
Please store the formatted message in
msgand raiseValueError(msg)instead of constructing it inline.Proposed fix
- raise ValueError( + msg = ( 'max_wallclock_seconds must be a positive integer (in seconds)! ' f"It is instead '{job_tmpl.max_wallclock_seconds}'" ) + raise ValueError(msg)As per coding guidelines,
**/*.pyfiles must “Assign exception messages to a variable before raising, e.g.msg = f'...'; raise TypeError(msg).”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/schedulers/plugins/slurm.py` around lines 365 - 366, In the validation branch for job_tmpl.max_wallclock_seconds, assign the complete formatted error text to a local variable named msg, then raise ValueError(msg) instead of constructing the message inline. Preserve the existing message content and validation behavior.Source: Coding guidelines
src/aiida/orm/nodes/data/remote/stash/custom.py (1)
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
source_listannotations consistent with accepted inputs.The setter and upstream validation accept both
listandtuple, but the constructor advertises onlylist[str]. This makes valid tuple callers fail static type checking. Use a shared type such aslist[str] | tuple[str, ...], or normalize tuples to lists at the boundary.Also applies to: 75-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/nodes/data/remote/stash/custom.py` at line 39, Update the source_list type annotations in the constructor and setter of the relevant remote stash class to accept both list[str] and tuple[str, ...], matching existing validation and accepted inputs. Alternatively, normalize tuples to lists at the constructor boundary, but keep annotations and runtime behavior consistent for all valid callers.src/aiida/orm/nodes/process/process.py (1)
178-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace tuple concatenation with iterable unpacking in both class properties.
src/aiida/orm/nodes/process/process.py#L178-L179: usereturn (*super()._hash_ignored_attributes, 'metadata_inputs').src/aiida/orm/nodes/process/workflow/workchain.py#L65-L66: usereturn (*super()._updatable_attributes, cls.STEPPER_STATE_INFO_KEY).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/nodes/process/process.py` around lines 178 - 179, The class properties must use iterable unpacking instead of tuple concatenation. In src/aiida/orm/nodes/process/process.py lines 178-179, update _hash_ignored_attributes to unpack super()._hash_ignored_attributes and append 'metadata_inputs'; in src/aiida/orm/nodes/process/workflow/workchain.py lines 65-66, update the corresponding updatable-attributes property to unpack super()._updatable_attributes and append cls.STEPPER_STATE_INFO_KEY.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aiida/orm/nodes/data/data.py`:
- Around line 171-172: In each changed exception branch in the relevant
data-format handling method, including the branches around the messages at
171–172, 278–279, and 334–335, assign the complete rewritten message to a local
variable named msg before raising. Then raise ValueError using msg, preserving
the existing message content and branch behavior.
In `@src/aiida/orm/nodes/process/calculation/calcjob.py`:
- Line 529: Update get_scheduler_stdout and the related scheduler output method
to annotate their return types as str | None instead of AnyStr | None, matching
the text returned by get_object_content without mode='rb'.
- Around line 108-116: Update the retrieval directive tuple annotations
throughout the affected fields and methods, including retrieve_list and
retrieve_temporary_list, from tuple[str, str, int] to tuple[str, str, int |
None]. Keep the existing sequence and optional field structure unchanged so the
types accurately match validator support for a None third element.
- Line 42: Update the return annotation of get_objects_to_hash from list[Any] to
a mutable mapping type such as dict[str, Any], matching its
objects.pop('repository_hash', None) usage.
In `@src/aiida/orm/nodes/process/process.py`:
- Line 88: Update the return annotation of the process node’s
get_objects_to_hash method to dict[str, Any], matching
NodeCaching.get_objects_to_hash and the mapping consumed by res.update(...).
In `@src/aiida/orm/utils/links.py`:
- Around line 199-203: In the duplicate-link validation branches of the
link-checking logic, including the branch guarded by outdegree ==
'unique_triple' and duplicate_link_triple and the additional branch noted in the
comment, assign each formatted exception message to a local variable named msg
before raising ValueError with that variable.
In `@src/aiida/parsers/plugins/templatereplacer/parser.py`:
- Line 58: Update retrieve_temporary_files before the open call to resolve each
requested path beneath retrieved_temporary_folder, reject absolute paths,
parent-directory traversal, and symlink-based escapes outside the resolved
temporary root, then open only the validated Path object. Preserve normal
in-root temporary-file retrieval.
In `@src/aiida/tools/data/array/kpoints/legacy.py`:
- Line 997: Update both occurrences that compute in-plane index permutations to
use an ordered comprehension rather than set subtraction, preserving the
deterministic coordinate order required when permutation is later applied.
---
Nitpick comments:
In `@src/aiida/engine/daemon/client.py`:
- Line 568: Update the get_client method annotation to return
Iterator[CircusClient], matching its contextmanager yield behavior, and import
Iterator from collections.abc if it is not already available.
In `@src/aiida/engine/processes/ports.py`:
- Line 262: Update the serialize method annotation to accept Mapping[str, Any] |
None instead of dict[str, Any] | None, matching its existing runtime support for
mapping inputs while leaving the implementation unchanged.
In `@src/aiida/orm/nodes/data/remote/stash/custom.py`:
- Line 39: Update the source_list type annotations in the constructor and setter
of the relevant remote stash class to accept both list[str] and tuple[str, ...],
matching existing validation and accepted inputs. Alternatively, normalize
tuples to lists at the constructor boundary, but keep annotations and runtime
behavior consistent for all valid callers.
In `@src/aiida/orm/nodes/process/process.py`:
- Around line 178-179: The class properties must use iterable unpacking instead
of tuple concatenation. In src/aiida/orm/nodes/process/process.py lines 178-179,
update _hash_ignored_attributes to unpack super()._hash_ignored_attributes and
append 'metadata_inputs'; in src/aiida/orm/nodes/process/workflow/workchain.py
lines 65-66, update the corresponding updatable-attributes property to unpack
super()._updatable_attributes and append cls.STEPPER_STATE_INFO_KEY.
In `@src/aiida/schedulers/plugins/slurm.py`:
- Around line 365-366: In the validation branch for
job_tmpl.max_wallclock_seconds, assign the complete formatted error text to a
local variable named msg, then raise ValueError(msg) instead of constructing the
message inline. Preserve the existing message content and validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6bdb95f9-d974-4150-95de-e627b8cecc51
📒 Files selected for processing (254)
.github/system_tests/test_daemon.py.molecule/default/files/polish/lib/workchain.pydocs/source/conf.pydocs/source/topics/processes/include/snippets/functions/typing_none.pydocs/source/topics/processes/include/snippets/functions/typing_union.pypyproject.tomlsrc/aiida/brokers/broker.pysrc/aiida/brokers/rabbitmq/broker.pysrc/aiida/brokers/rabbitmq/client.pysrc/aiida/brokers/zeromq/broker.pysrc/aiida/brokers/zeromq/communicator.pysrc/aiida/brokers/zeromq/server.pysrc/aiida/calculations/diff_tutorial/calculations.pysrc/aiida/calculations/importers/arithmetic/add.pysrc/aiida/calculations/stash.pysrc/aiida/calculations/templatereplacer.pysrc/aiida/cmdline/commands/cmd_bug_report.pysrc/aiida/cmdline/commands/cmd_calcjob.pysrc/aiida/cmdline/commands/cmd_code.pysrc/aiida/cmdline/commands/cmd_config.pysrc/aiida/cmdline/commands/cmd_node.pysrc/aiida/cmdline/commands/cmd_process.pysrc/aiida/cmdline/commands/cmd_rabbitmq.pysrc/aiida/cmdline/commands/cmd_storage.pysrc/aiida/cmdline/groups/verdi.pysrc/aiida/cmdline/params/options/callable.pysrc/aiida/cmdline/params/options/main.pysrc/aiida/cmdline/params/types/code.pysrc/aiida/cmdline/params/types/identifier.pysrc/aiida/cmdline/params/types/path.pysrc/aiida/cmdline/params/types/plugin.pysrc/aiida/cmdline/utils/common.pysrc/aiida/cmdline/utils/echo.pysrc/aiida/common/folders.pysrc/aiida/common/hashing.pysrc/aiida/common/lang.pysrc/aiida/common/progress_reporter.pysrc/aiida/common/timezone.pysrc/aiida/common/typing.pysrc/aiida/common/utils.pysrc/aiida/engine/daemon/client.pysrc/aiida/engine/daemon/execmanager.pysrc/aiida/engine/daemon/worker.pysrc/aiida/engine/launch.pysrc/aiida/engine/persistence.pysrc/aiida/engine/processes/builder.pysrc/aiida/engine/processes/calcjobs/calcjob.pysrc/aiida/engine/processes/calcjobs/importer.pysrc/aiida/engine/processes/calcjobs/manager.pysrc/aiida/engine/processes/calcjobs/tasks.pysrc/aiida/engine/processes/control.pysrc/aiida/engine/processes/exit_code.pysrc/aiida/engine/processes/functions.pysrc/aiida/engine/processes/futures.pysrc/aiida/engine/processes/ports.pysrc/aiida/engine/processes/process.pysrc/aiida/engine/processes/process_spec.pysrc/aiida/engine/processes/workchains/awaitable.pysrc/aiida/engine/processes/workchains/context.pysrc/aiida/engine/processes/workchains/restart.pysrc/aiida/engine/processes/workchains/utils.pysrc/aiida/engine/processes/workchains/workchain.pysrc/aiida/engine/runners.pysrc/aiida/engine/transports.pysrc/aiida/engine/utils.pysrc/aiida/manage/configuration/__init__.pysrc/aiida/manage/configuration/config.pysrc/aiida/manage/configuration/migrations/migrations.pysrc/aiida/manage/configuration/options.pysrc/aiida/manage/configuration/profile.pysrc/aiida/manage/manager.pysrc/aiida/manage/profile_access.pysrc/aiida/manage/tests/pytest_fixtures.pysrc/aiida/orm/authinfos.pysrc/aiida/orm/comments.pysrc/aiida/orm/computers.pysrc/aiida/orm/entities.pysrc/aiida/orm/extras.pysrc/aiida/orm/fields.pysrc/aiida/orm/groups.pysrc/aiida/orm/implementation/authinfos.pysrc/aiida/orm/implementation/comments.pysrc/aiida/orm/implementation/computers.pysrc/aiida/orm/implementation/entities.pysrc/aiida/orm/implementation/groups.pysrc/aiida/orm/implementation/logs.pysrc/aiida/orm/implementation/nodes.pysrc/aiida/orm/implementation/querybuilder.pysrc/aiida/orm/implementation/storage_backend.pysrc/aiida/orm/logs.pysrc/aiida/orm/nodes/attributes.pysrc/aiida/orm/nodes/caching.pysrc/aiida/orm/nodes/comments.pysrc/aiida/orm/nodes/data/array/array.pysrc/aiida/orm/nodes/data/array/bands.pysrc/aiida/orm/nodes/data/array/kpoints.pysrc/aiida/orm/nodes/data/array/trajectory.pysrc/aiida/orm/nodes/data/array/xy.pysrc/aiida/orm/nodes/data/cif.pysrc/aiida/orm/nodes/data/code/abstract.pysrc/aiida/orm/nodes/data/code/legacy.pysrc/aiida/orm/nodes/data/data.pysrc/aiida/orm/nodes/data/dict.pysrc/aiida/orm/nodes/data/enum.pysrc/aiida/orm/nodes/data/remote/base.pysrc/aiida/orm/nodes/data/remote/stash/compress.pysrc/aiida/orm/nodes/data/remote/stash/custom.pysrc/aiida/orm/nodes/data/remote/stash/folder.pysrc/aiida/orm/nodes/data/structure.pysrc/aiida/orm/nodes/data/upf.pysrc/aiida/orm/nodes/links.pysrc/aiida/orm/nodes/node.pysrc/aiida/orm/nodes/process/calculation/calcjob.pysrc/aiida/orm/nodes/process/process.pysrc/aiida/orm/nodes/process/workflow/workchain.pysrc/aiida/orm/nodes/process/workflow/workflow.pysrc/aiida/orm/nodes/repository.pysrc/aiida/orm/pydantic.pysrc/aiida/orm/querybuilder.pysrc/aiida/orm/users.pysrc/aiida/orm/utils/builders/code.pysrc/aiida/orm/utils/calcjob.pysrc/aiida/orm/utils/links.pysrc/aiida/orm/utils/managers.pysrc/aiida/orm/utils/mixins.pysrc/aiida/orm/utils/serialize.pysrc/aiida/parsers/parser.pysrc/aiida/parsers/plugins/templatereplacer/parser.pysrc/aiida/plugins/entry_point.pysrc/aiida/plugins/factories.pysrc/aiida/repository/backend/abstract.pysrc/aiida/repository/backend/disk_object_store.pysrc/aiida/repository/common.pysrc/aiida/repository/repository.pysrc/aiida/restapi/translator/base.pysrc/aiida/restapi/translator/nodes/data/__init__.pysrc/aiida/schedulers/plugins/lsf.pysrc/aiida/schedulers/plugins/pbsbaseclasses.pysrc/aiida/schedulers/plugins/pbspro.pysrc/aiida/schedulers/plugins/sge.pysrc/aiida/schedulers/plugins/slurm.pysrc/aiida/schedulers/plugins/torque.pysrc/aiida/schedulers/scheduler.pysrc/aiida/storage/psql_dos/backend.pysrc/aiida/storage/psql_dos/migrations/utils/dblog_update.pysrc/aiida/storage/psql_dos/migrations/utils/integrity.pysrc/aiida/storage/psql_dos/migrations/utils/provenance_redesign.pysrc/aiida/storage/psql_dos/migrations/utils/utils.pysrc/aiida/storage/psql_dos/migrator.pysrc/aiida/storage/psql_dos/orm/entities.pysrc/aiida/storage/psql_dos/orm/extras_mixin.pysrc/aiida/storage/psql_dos/orm/nodes.pysrc/aiida/storage/psql_dos/orm/querybuilder/joiner.pysrc/aiida/storage/psql_dos/orm/querybuilder/main.pysrc/aiida/storage/psql_dos/orm/utils.pysrc/aiida/storage/sqlite_dos/backend.pysrc/aiida/storage/sqlite_temp/backend.pysrc/aiida/storage/sqlite_zip/backend.pysrc/aiida/storage/sqlite_zip/migrations/legacy/__init__.pysrc/aiida/storage/sqlite_zip/migrations/legacy/v05_to_v06.pysrc/aiida/storage/sqlite_zip/migrations/legacy_to_main.pysrc/aiida/storage/sqlite_zip/migrations/utils.pysrc/aiida/storage/sqlite_zip/migrator.pysrc/aiida/storage/sqlite_zip/models.pysrc/aiida/storage/sqlite_zip/orm.pysrc/aiida/storage/sqlite_zip/utils.pysrc/aiida/storage/utils.pysrc/aiida/tools/_dumping/config.pysrc/aiida/tools/_dumping/detect.pysrc/aiida/tools/_dumping/engine.pysrc/aiida/tools/_dumping/executors/collection.pysrc/aiida/tools/_dumping/executors/deletion.pysrc/aiida/tools/_dumping/executors/process.pysrc/aiida/tools/_dumping/mapping.pysrc/aiida/tools/_dumping/tracking.pysrc/aiida/tools/_dumping/utils.pysrc/aiida/tools/archive/abstract.pysrc/aiida/tools/archive/common.pysrc/aiida/tools/archive/create.pysrc/aiida/tools/archive/implementations/sqlite_zip/main.pysrc/aiida/tools/archive/implementations/sqlite_zip/reader.pysrc/aiida/tools/archive/implementations/sqlite_zip/writer.pysrc/aiida/tools/archive/imports.pysrc/aiida/tools/data/array/kpoints/legacy.pysrc/aiida/tools/dbimporters/baseclasses.pysrc/aiida/tools/dbimporters/plugins/icsd.pysrc/aiida/tools/dbimporters/plugins/materialsproject.pysrc/aiida/tools/dbimporters/plugins/nninc.pysrc/aiida/tools/graph/age_entities.pysrc/aiida/tools/graph/age_rules.pysrc/aiida/tools/graph/deletions.pysrc/aiida/tools/graph/graph_traversers.pysrc/aiida/tools/groups/paths.pysrc/aiida/tools/ipython/ipython_magics.pysrc/aiida/tools/pytest_fixtures/configuration.pysrc/aiida/tools/pytest_fixtures/daemon.pysrc/aiida/tools/pytest_fixtures/entry_points.pysrc/aiida/tools/pytest_fixtures/orm.pysrc/aiida/tools/pytest_fixtures/storage.pysrc/aiida/tools/query/mapping.pysrc/aiida/tools/visualization/graph.pysrc/aiida/transports/cli.pysrc/aiida/transports/plugins/async_backend.pysrc/aiida/transports/plugins/local.pysrc/aiida/transports/plugins/ssh.pysrc/aiida/transports/plugins/ssh_async.pysrc/aiida/transports/transport.pytests/calculations/test_transfer.pytests/cmdline/commands/test_archive_import.pytests/cmdline/commands/test_bug_report.pytests/cmdline/commands/test_data.pytests/cmdline/commands/test_node.pytests/cmdline/commands/test_storage.pytests/cmdline/groups/test_dynamic.pytests/cmdline/params/types/test_path.pytests/common/test_extendeddicts.pytests/common/test_hashing.pytests/conftest.pytests/engine/processes/calcjobs/test_calc_job.pytests/engine/test_process.pytests/engine/test_process_function.pytests/manage/configuration/test_config.pytests/orm/implementation/test_comments.pytests/orm/implementation/test_logs.pytests/orm/nodes/data/test_data.pytests/orm/nodes/data/test_upf.pytests/orm/nodes/test_node.pytests/orm/test_autogroups.pytests/orm/test_entities.pytests/orm/test_groups.pytests/orm/test_querybuilder.pytests/orm/utils/test_managers.pytests/repository/backend/test_abstract.pytests/repository/conftest.pytests/repository/test_repository.pytests/restapi/conftest.pytests/restapi/test_identifiers.pytests/storage/psql_dos/migrations/django_branch/test_0047_migrate_repository.pytests/storage/psql_dos/migrations/sqlalchemy_branch/test_11_v2_repository.pytests/storage/psql_dos/test_schema.pytests/test_dataclasses.pytests/test_dbimporters.pytests/test_nodes.pytests/tools/archive/orm/test_computers.pytests/tools/archive/orm/test_groups.pytests/tools/archive/orm/test_links.pytests/tools/dumping/integration_tests.pytests/tools/dumping/utils.pytests/tools/graph/test_age.pytests/tools/graph/test_graph_traversers.pytests/tools/visualization/test_graph.pytests/transports/test_all_plugins.pytests/utils/archives.pyutils/autogenerate_all_imports.py
🚧 Files skipped from review as they are similar to previous changes (206)
- src/aiida/tools/dbimporters/plugins/nninc.py
- tests/test_dbimporters.py
- src/aiida/brokers/rabbitmq/client.py
- src/aiida/storage/sqlite_zip/migrations/utils.py
- src/aiida/engine/processes/workchains/workchain.py
- src/aiida/cmdline/utils/common.py
- src/aiida/orm/utils/calcjob.py
- src/aiida/restapi/translator/nodes/data/init.py
- src/aiida/cmdline/params/types/identifier.py
- src/aiida/common/folders.py
- src/aiida/tools/dbimporters/plugins/materialsproject.py
- tests/cmdline/params/types/test_path.py
- src/aiida/orm/nodes/process/workflow/workflow.py
- src/aiida/orm/utils/builders/code.py
- src/aiida/calculations/stash.py
- src/aiida/orm/utils/mixins.py
- tests/orm/nodes/data/test_data.py
- src/aiida/engine/daemon/worker.py
- src/aiida/orm/pydantic.py
- src/aiida/orm/implementation/logs.py
- src/aiida/common/lang.py
- src/aiida/tools/pytest_fixtures/daemon.py
- src/aiida/calculations/templatereplacer.py
- tests/orm/nodes/test_node.py
- src/aiida/orm/nodes/data/array/array.py
- src/aiida/engine/processes/workchains/context.py
- src/aiida/engine/processes/workchains/awaitable.py
- src/aiida/cmdline/params/options/callable.py
- src/aiida/schedulers/plugins/pbspro.py
- src/aiida/tools/dbimporters/plugins/icsd.py
- src/aiida/repository/common.py
- src/aiida/cmdline/params/types/plugin.py
- tests/repository/test_repository.py
- src/aiida/storage/sqlite_zip/models.py
- src/aiida/storage/psql_dos/orm/entities.py
- tests/cmdline/commands/test_node.py
- src/aiida/orm/nodes/data/dict.py
- src/aiida/tools/archive/common.py
- src/aiida/orm/implementation/groups.py
- src/aiida/transports/cli.py
- src/aiida/storage/sqlite_zip/migrations/legacy/init.py
- src/aiida/orm/implementation/computers.py
- tests/tools/archive/orm/test_computers.py
- src/aiida/brokers/zeromq/communicator.py
- src/aiida/engine/processes/workchains/utils.py
- src/aiida/cmdline/commands/cmd_storage.py
- src/aiida/orm/nodes/data/upf.py
- src/aiida/orm/nodes/data/enum.py
- src/aiida/cmdline/params/types/code.py
- pyproject.toml
- src/aiida/brokers/broker.py
- src/aiida/orm/utils/managers.py
- src/aiida/orm/nodes/data/code/legacy.py
- src/aiida/brokers/zeromq/server.py
- src/aiida/tools/query/mapping.py
- src/aiida/engine/transports.py
- src/aiida/cmdline/commands/cmd_rabbitmq.py
- src/aiida/storage/psql_dos/migrations/utils/dblog_update.py
- src/aiida/cmdline/commands/cmd_calcjob.py
- src/aiida/schedulers/scheduler.py
- src/aiida/cmdline/commands/cmd_bug_report.py
- src/aiida/storage/psql_dos/migrations/utils/provenance_redesign.py
- src/aiida/engine/processes/futures.py
- src/aiida/engine/processes/process_spec.py
- src/aiida/cmdline/params/options/main.py
- src/aiida/orm/nodes/data/remote/base.py
- tests/orm/implementation/test_comments.py
- src/aiida/engine/processes/control.py
- docs/source/topics/processes/include/snippets/functions/typing_none.py
- src/aiida/orm/nodes/data/array/xy.py
- src/aiida/common/utils.py
- src/aiida/cmdline/utils/echo.py
- src/aiida/orm/implementation/authinfos.py
- src/aiida/engine/processes/exit_code.py
- src/aiida/manage/configuration/options.py
- src/aiida/storage/psql_dos/migrations/utils/integrity.py
- docs/source/conf.py
- src/aiida/orm/nodes/caching.py
- src/aiida/orm/nodes/data/array/bands.py
- tests/storage/psql_dos/test_schema.py
- src/aiida/storage/sqlite_temp/backend.py
- tests/tools/archive/orm/test_groups.py
- src/aiida/orm/nodes/data/cif.py
- tests/storage/psql_dos/migrations/django_branch/test_0047_migrate_repository.py
- src/aiida/schedulers/plugins/pbsbaseclasses.py
- src/aiida/orm/nodes/attributes.py
- src/aiida/cmdline/commands/cmd_node.py
- src/aiida/engine/launch.py
- src/aiida/tools/archive/implementations/sqlite_zip/main.py
- src/aiida/tools/_dumping/engine.py
- src/aiida/tools/graph/graph_traversers.py
- tests/cmdline/groups/test_dynamic.py
- src/aiida/storage/psql_dos/orm/nodes.py
- src/aiida/engine/processes/builder.py
- tests/cmdline/commands/test_data.py
- src/aiida/tools/archive/implementations/sqlite_zip/reader.py
- tests/conftest.py
- tests/tools/dumping/utils.py
- tests/cmdline/commands/test_archive_import.py
- tests/utils/archives.py
- src/aiida/engine/processes/calcjobs/importer.py
- src/aiida/engine/persistence.py
- src/aiida/common/progress_reporter.py
- src/aiida/orm/nodes/comments.py
- src/aiida/schedulers/plugins/sge.py
- src/aiida/orm/nodes/links.py
- src/aiida/orm/nodes/repository.py
- src/aiida/storage/psql_dos/migrator.py
- tests/storage/psql_dos/migrations/sqlalchemy_branch/test_11_v2_repository.py
- src/aiida/tools/visualization/graph.py
- src/aiida/calculations/importers/arithmetic/add.py
- src/aiida/storage/utils.py
- src/aiida/orm/nodes/data/remote/stash/folder.py
- src/aiida/tools/dbimporters/baseclasses.py
- tests/restapi/test_identifiers.py
- src/aiida/schedulers/plugins/torque.py
- src/aiida/tools/groups/paths.py
- tests/common/test_hashing.py
- tests/common/test_extendeddicts.py
- src/aiida/storage/psql_dos/migrations/utils/utils.py
- tests/manage/configuration/test_config.py
- tests/transports/test_all_plugins.py
- src/aiida/restapi/translator/base.py
- src/aiida/tools/graph/age_rules.py
- src/aiida/common/timezone.py
- src/aiida/orm/users.py
- src/aiida/schedulers/plugins/lsf.py
- src/aiida/engine/processes/functions.py
- src/aiida/common/typing.py
- src/aiida/orm/nodes/data/array/trajectory.py
- src/aiida/manage/configuration/init.py
- tests/tools/visualization/test_graph.py
- src/aiida/storage/sqlite_zip/migrations/legacy/v05_to_v06.py
- src/aiida/manage/configuration/migrations/migrations.py
- src/aiida/tools/graph/age_entities.py
- src/aiida/tools/pytest_fixtures/storage.py
- tests/cmdline/commands/test_bug_report.py
- src/aiida/tools/_dumping/mapping.py
- src/aiida/parsers/parser.py
- src/aiida/repository/backend/abstract.py
- src/aiida/tools/_dumping/executors/deletion.py
- src/aiida/storage/sqlite_zip/orm.py
- src/aiida/manage/configuration/profile.py
- src/aiida/brokers/rabbitmq/broker.py
- docs/source/topics/processes/include/snippets/functions/typing_union.py
- tests/repository/backend/test_abstract.py
- src/aiida/manage/profile_access.py
- utils/autogenerate_all_imports.py
- src/aiida/orm/extras.py
- tests/orm/utils/test_managers.py
- src/aiida/engine/daemon/execmanager.py
- src/aiida/manage/configuration/config.py
- src/aiida/engine/processes/workchains/restart.py
- src/aiida/plugins/entry_point.py
- tests/orm/test_querybuilder.py
- src/aiida/cmdline/commands/cmd_config.py
- src/aiida/transports/plugins/local.py
- tests/orm/nodes/data/test_upf.py
- tests/engine/test_process.py
- src/aiida/orm/entities.py
- src/aiida/storage/psql_dos/backend.py
- src/aiida/orm/nodes/data/array/kpoints.py
- src/aiida/orm/implementation/entities.py
- src/aiida/orm/authinfos.py
- src/aiida/orm/implementation/nodes.py
- src/aiida/transports/plugins/ssh.py
- src/aiida/tools/archive/create.py
- src/aiida/tools/_dumping/detect.py
- src/aiida/repository/backend/disk_object_store.py
- src/aiida/storage/sqlite_zip/migrator.py
- tests/orm/implementation/test_logs.py
- tests/tools/dumping/integration_tests.py
- src/aiida/tools/_dumping/executors/collection.py
- src/aiida/transports/plugins/async_backend.py
- src/aiida/engine/processes/calcjobs/manager.py
- src/aiida/storage/psql_dos/orm/extras_mixin.py
- src/aiida/cmdline/params/types/path.py
- src/aiida/storage/sqlite_zip/migrations/legacy_to_main.py
- .github/system_tests/test_daemon.py
- tests/test_dataclasses.py
- src/aiida/common/hashing.py
- src/aiida/engine/runners.py
- src/aiida/orm/groups.py
- tests/tools/graph/test_graph_traversers.py
- tests/engine/test_process_function.py
- src/aiida/storage/psql_dos/orm/querybuilder/joiner.py
- src/aiida/orm/fields.py
- src/aiida/tools/_dumping/config.py
- src/aiida/tools/_dumping/tracking.py
- tests/orm/test_groups.py
- src/aiida/orm/nodes/data/structure.py
- src/aiida/orm/nodes/node.py
- src/aiida/tools/archive/abstract.py
- src/aiida/engine/processes/process.py
- src/aiida/repository/repository.py
- src/aiida/orm/computers.py
- tests/tools/graph/test_age.py
- src/aiida/engine/utils.py
- src/aiida/transports/transport.py
- src/aiida/tools/_dumping/utils.py
- src/aiida/transports/plugins/ssh_async.py
- src/aiida/manage/manager.py
- src/aiida/engine/processes/calcjobs/calcjob.py
- src/aiida/tools/archive/imports.py
- src/aiida/storage/psql_dos/orm/querybuilder/main.py
- src/aiida/orm/querybuilder.py
de39b86 to
670b900
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/aiida/orm/computers.py (1)
43-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the documented tuple order.
get_or_createreturns(created, computer)at Lines 56 and 58, matchingtuple[bool, Computer], but the docstring documents(computer, created). Update the:return:description to prevent callers from misinterpreting the values.Proposed fix
- :return: (computer, created) where computer is the computer (new or existing, - in any case already stored) and created is a boolean saying + :return: (created, computer) where created indicates whether a new computer + was created and computer is the new or existing stored computer🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/computers.py` around lines 43 - 49, Update the :return: description in get_or_create to document the tuple as (created, computer), matching its tuple[bool, Computer] annotation and both return statements. Keep the existing descriptions of the values otherwise unchanged.src/aiida/orm/fields.py (1)
18-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIgnore
NoneTypewhen unwrapping unions.None | strcurrently resolves toQbAnyFieldinstead ofQbStrFieldbecauseextract_root_typerecurses into the first union arg. DropNoneTypebefore recursing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/fields.py` around lines 18 - 40, Update extract_root_type’s union-unwrapping logic to exclude NoneType from the union arguments before recursively resolving the root type. Preserve the existing behavior for non-optional unions and ensure annotations such as None | str resolve to the string root type rather than QbAnyField.
🧹 Nitpick comments (2)
src/aiida/schedulers/plugins/torque.py (1)
70-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssign the exception message before raising.
The f-string preserves the message, but repository guidelines require exception messages to be assigned to a variable first.
As per coding guidelines, assign exception messages to a variable before raising.
Proposed fix
except ValueError: - raise ValueError( + msg = ( 'max_wallclock_seconds must be a positive integer (in seconds)! ' f"It is instead '{max_wallclock_seconds}'" ) + raise ValueError(msg)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/schedulers/plugins/torque.py` around lines 70 - 73, In the validation path for max_wallclock_seconds, assign the complete ValueError message to a local variable before the raise statement, then raise ValueError using that variable while preserving the existing message text.Source: Coding guidelines
src/aiida/orm/nodes/process/process.py (1)
179-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse tuple unpacking instead of tuple concatenation.
Both class properties trigger Ruff RUF005. Replace concatenation with tuple unpacking while preserving the tuple return type.
src/aiida/orm/nodes/process/process.py#L179-L179: usereturn (*super()._hash_ignored_attributes, 'metadata_inputs').src/aiida/orm/nodes/process/workflow/workchain.py#L66-L66: usereturn (*super()._updatable_attributes, cls.STEPPER_STATE_INFO_KEY).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/nodes/process/process.py` at line 179, Replace tuple concatenation with tuple unpacking in the class properties at src/aiida/orm/nodes/process/process.py lines 179-179 and src/aiida/orm/nodes/process/workflow/workchain.py lines 66-66: update the _hash_ignored_attributes return to unpack super()._hash_ignored_attributes and append 'metadata_inputs', and update the _updatable_attributes return to unpack super()._updatable_attributes and append cls.STEPPER_STATE_INFO_KEY, preserving tuple return types.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aiida/engine/processes/calcjobs/manager.py`:
- Line 170: Update the future annotations to use JobInfo | None wherever the
nullable result is declared: manager.py lines 64-64, 170-170 in
request_job_info_update, and 292-292. Keep the existing iterator and future
structure unchanged while propagating the nullable JobInfo type at all three
sites.
In `@src/aiida/manage/manager.py`:
- Line 383: Update Manager.get_communicator() to annotate its return value with
the shared communicator interface, or a union covering RmqThreadCommunicator and
ZeromqCommunicator, so the type reflects either configured backend.
In `@src/aiida/orm/computers.py`:
- Around line 474-488: Align get_mpirun_command with set_mpirun_command’s tuple
support: either normalize tuple inputs to lists in set_mpirun_command before
storing them, or update the getter annotation and contract to return list[str] |
tuple[str, ...]. Keep the default command and existing string validation
unchanged.
In `@src/aiida/orm/nodes/data/structure.py`:
- Around line 1460-1465: Assign each formatted ValueError message to msg before
raising it: update the explicit kind-name conflict path around structure.py
lines 1460-1465, the invalid kind input path around lines 1999-2003, and the
invalid ase input path around lines 2020-2024 to raise ValueError(msg) while
preserving the existing message text.
In `@src/aiida/tools/graph/age_entities.py`:
- Around line 212-214: Update both ValueError paths in the input validation
logic, including the input_object block, to build the concatenated exception
text in a local msg variable first, then raise ValueError(msg). Preserve the
existing message content and validation behavior.
---
Outside diff comments:
In `@src/aiida/orm/computers.py`:
- Around line 43-49: Update the :return: description in get_or_create to
document the tuple as (created, computer), matching its tuple[bool, Computer]
annotation and both return statements. Keep the existing descriptions of the
values otherwise unchanged.
In `@src/aiida/orm/fields.py`:
- Around line 18-40: Update extract_root_type’s union-unwrapping logic to
exclude NoneType from the union arguments before recursively resolving the root
type. Preserve the existing behavior for non-optional unions and ensure
annotations such as None | str resolve to the string root type rather than
QbAnyField.
---
Nitpick comments:
In `@src/aiida/orm/nodes/process/process.py`:
- Line 179: Replace tuple concatenation with tuple unpacking in the class
properties at src/aiida/orm/nodes/process/process.py lines 179-179 and
src/aiida/orm/nodes/process/workflow/workchain.py lines 66-66: update the
_hash_ignored_attributes return to unpack super()._hash_ignored_attributes and
append 'metadata_inputs', and update the _updatable_attributes return to unpack
super()._updatable_attributes and append cls.STEPPER_STATE_INFO_KEY, preserving
tuple return types.
In `@src/aiida/schedulers/plugins/torque.py`:
- Around line 70-73: In the validation path for max_wallclock_seconds, assign
the complete ValueError message to a local variable before the raise statement,
then raise ValueError using that variable while preserving the existing message
text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cfd74c61-1847-47ac-bbb1-f486af187460
📒 Files selected for processing (232)
.github/system_tests/test_daemon.py.molecule/default/files/polish/lib/workchain.pydocs/source/conf.pydocs/source/topics/processes/include/snippets/functions/typing_none.pydocs/source/topics/processes/include/snippets/functions/typing_union.pypyproject.tomlsrc/aiida/brokers/broker.pysrc/aiida/brokers/rabbitmq/broker.pysrc/aiida/brokers/rabbitmq/client.pysrc/aiida/brokers/zeromq/broker.pysrc/aiida/brokers/zeromq/communicator.pysrc/aiida/brokers/zeromq/server.pysrc/aiida/calculations/diff_tutorial/calculations.pysrc/aiida/calculations/importers/arithmetic/add.pysrc/aiida/calculations/stash.pysrc/aiida/calculations/templatereplacer.pysrc/aiida/cmdline/commands/cmd_bug_report.pysrc/aiida/cmdline/commands/cmd_calcjob.pysrc/aiida/cmdline/commands/cmd_code.pysrc/aiida/cmdline/commands/cmd_config.pysrc/aiida/cmdline/commands/cmd_rabbitmq.pysrc/aiida/cmdline/commands/cmd_storage.pysrc/aiida/cmdline/groups/verdi.pysrc/aiida/cmdline/params/options/callable.pysrc/aiida/cmdline/params/options/main.pysrc/aiida/cmdline/params/types/code.pysrc/aiida/cmdline/params/types/identifier.pysrc/aiida/cmdline/params/types/path.pysrc/aiida/cmdline/params/types/plugin.pysrc/aiida/cmdline/utils/common.pysrc/aiida/cmdline/utils/echo.pysrc/aiida/common/folders.pysrc/aiida/common/hashing.pysrc/aiida/common/lang.pysrc/aiida/common/progress_reporter.pysrc/aiida/common/timezone.pysrc/aiida/common/typing.pysrc/aiida/common/utils.pysrc/aiida/engine/daemon/client.pysrc/aiida/engine/daemon/execmanager.pysrc/aiida/engine/daemon/worker.pysrc/aiida/engine/launch.pysrc/aiida/engine/persistence.pysrc/aiida/engine/processes/builder.pysrc/aiida/engine/processes/calcjobs/calcjob.pysrc/aiida/engine/processes/calcjobs/importer.pysrc/aiida/engine/processes/calcjobs/manager.pysrc/aiida/engine/processes/calcjobs/tasks.pysrc/aiida/engine/processes/control.pysrc/aiida/engine/processes/exit_code.pysrc/aiida/engine/processes/functions.pysrc/aiida/engine/processes/futures.pysrc/aiida/engine/processes/ports.pysrc/aiida/engine/processes/process.pysrc/aiida/engine/processes/process_spec.pysrc/aiida/engine/processes/workchains/awaitable.pysrc/aiida/engine/processes/workchains/context.pysrc/aiida/engine/processes/workchains/restart.pysrc/aiida/engine/processes/workchains/utils.pysrc/aiida/engine/processes/workchains/workchain.pysrc/aiida/engine/runners.pysrc/aiida/engine/transports.pysrc/aiida/engine/utils.pysrc/aiida/manage/configuration/__init__.pysrc/aiida/manage/configuration/config.pysrc/aiida/manage/configuration/migrations/migrations.pysrc/aiida/manage/configuration/options.pysrc/aiida/manage/configuration/profile.pysrc/aiida/manage/manager.pysrc/aiida/manage/profile_access.pysrc/aiida/manage/tests/pytest_fixtures.pysrc/aiida/orm/authinfos.pysrc/aiida/orm/comments.pysrc/aiida/orm/computers.pysrc/aiida/orm/entities.pysrc/aiida/orm/extras.pysrc/aiida/orm/fields.pysrc/aiida/orm/groups.pysrc/aiida/orm/implementation/authinfos.pysrc/aiida/orm/implementation/comments.pysrc/aiida/orm/implementation/computers.pysrc/aiida/orm/implementation/entities.pysrc/aiida/orm/implementation/groups.pysrc/aiida/orm/implementation/logs.pysrc/aiida/orm/implementation/nodes.pysrc/aiida/orm/implementation/querybuilder.pysrc/aiida/orm/implementation/storage_backend.pysrc/aiida/orm/logs.pysrc/aiida/orm/nodes/attributes.pysrc/aiida/orm/nodes/caching.pysrc/aiida/orm/nodes/comments.pysrc/aiida/orm/nodes/data/array/array.pysrc/aiida/orm/nodes/data/array/bands.pysrc/aiida/orm/nodes/data/array/kpoints.pysrc/aiida/orm/nodes/data/array/trajectory.pysrc/aiida/orm/nodes/data/array/xy.pysrc/aiida/orm/nodes/data/cif.pysrc/aiida/orm/nodes/data/code/abstract.pysrc/aiida/orm/nodes/data/code/legacy.pysrc/aiida/orm/nodes/data/data.pysrc/aiida/orm/nodes/data/dict.pysrc/aiida/orm/nodes/data/enum.pysrc/aiida/orm/nodes/data/remote/base.pysrc/aiida/orm/nodes/data/remote/stash/compress.pysrc/aiida/orm/nodes/data/remote/stash/custom.pysrc/aiida/orm/nodes/data/remote/stash/folder.pysrc/aiida/orm/nodes/data/structure.pysrc/aiida/orm/nodes/data/upf.pysrc/aiida/orm/nodes/links.pysrc/aiida/orm/nodes/node.pysrc/aiida/orm/nodes/process/calculation/calcjob.pysrc/aiida/orm/nodes/process/process.pysrc/aiida/orm/nodes/process/workflow/workchain.pysrc/aiida/orm/nodes/process/workflow/workflow.pysrc/aiida/orm/nodes/repository.pysrc/aiida/orm/pydantic.pysrc/aiida/orm/querybuilder.pysrc/aiida/orm/users.pysrc/aiida/orm/utils/calcjob.pysrc/aiida/orm/utils/links.pysrc/aiida/orm/utils/managers.pysrc/aiida/orm/utils/mixins.pysrc/aiida/orm/utils/serialize.pysrc/aiida/parsers/parser.pysrc/aiida/parsers/plugins/templatereplacer/parser.pysrc/aiida/plugins/entry_point.pysrc/aiida/plugins/factories.pysrc/aiida/repository/backend/abstract.pysrc/aiida/repository/backend/disk_object_store.pysrc/aiida/repository/common.pysrc/aiida/repository/repository.pysrc/aiida/restapi/translator/base.pysrc/aiida/restapi/translator/nodes/data/__init__.pysrc/aiida/schedulers/plugins/lsf.pysrc/aiida/schedulers/plugins/pbspro.pysrc/aiida/schedulers/plugins/sge.pysrc/aiida/schedulers/plugins/slurm.pysrc/aiida/schedulers/plugins/torque.pysrc/aiida/schedulers/scheduler.pysrc/aiida/storage/psql_dos/backend.pysrc/aiida/storage/psql_dos/migrations/utils/dblog_update.pysrc/aiida/storage/psql_dos/migrations/utils/integrity.pysrc/aiida/storage/psql_dos/migrations/utils/utils.pysrc/aiida/storage/psql_dos/migrator.pysrc/aiida/storage/psql_dos/orm/entities.pysrc/aiida/storage/psql_dos/orm/extras_mixin.pysrc/aiida/storage/psql_dos/orm/nodes.pysrc/aiida/storage/psql_dos/orm/querybuilder/joiner.pysrc/aiida/storage/psql_dos/orm/querybuilder/main.pysrc/aiida/storage/sqlite_dos/backend.pysrc/aiida/storage/sqlite_temp/backend.pysrc/aiida/storage/sqlite_zip/backend.pysrc/aiida/storage/sqlite_zip/migrations/legacy/__init__.pysrc/aiida/storage/sqlite_zip/migrations/legacy/v05_to_v06.pysrc/aiida/storage/sqlite_zip/migrations/legacy_to_main.pysrc/aiida/storage/sqlite_zip/migrations/utils.pysrc/aiida/storage/sqlite_zip/migrator.pysrc/aiida/storage/sqlite_zip/models.pysrc/aiida/storage/sqlite_zip/orm.pysrc/aiida/storage/sqlite_zip/utils.pysrc/aiida/storage/utils.pysrc/aiida/tools/_dumping/config.pysrc/aiida/tools/_dumping/detect.pysrc/aiida/tools/_dumping/engine.pysrc/aiida/tools/_dumping/executors/collection.pysrc/aiida/tools/_dumping/executors/deletion.pysrc/aiida/tools/_dumping/executors/process.pysrc/aiida/tools/_dumping/mapping.pysrc/aiida/tools/_dumping/tracking.pysrc/aiida/tools/_dumping/utils.pysrc/aiida/tools/archive/abstract.pysrc/aiida/tools/archive/common.pysrc/aiida/tools/archive/create.pysrc/aiida/tools/archive/implementations/sqlite_zip/main.pysrc/aiida/tools/archive/implementations/sqlite_zip/reader.pysrc/aiida/tools/archive/implementations/sqlite_zip/writer.pysrc/aiida/tools/archive/imports.pysrc/aiida/tools/dbimporters/baseclasses.pysrc/aiida/tools/dbimporters/plugins/icsd.pysrc/aiida/tools/dbimporters/plugins/materialsproject.pysrc/aiida/tools/graph/age_entities.pysrc/aiida/tools/graph/age_rules.pysrc/aiida/tools/graph/deletions.pysrc/aiida/tools/graph/graph_traversers.pysrc/aiida/tools/groups/paths.pysrc/aiida/tools/ipython/ipython_magics.pysrc/aiida/tools/pytest_fixtures/configuration.pysrc/aiida/tools/pytest_fixtures/daemon.pysrc/aiida/tools/pytest_fixtures/entry_points.pysrc/aiida/tools/pytest_fixtures/orm.pysrc/aiida/tools/pytest_fixtures/storage.pysrc/aiida/tools/query/mapping.pysrc/aiida/tools/visualization/graph.pysrc/aiida/transports/cli.pysrc/aiida/transports/plugins/async_backend.pysrc/aiida/transports/plugins/local.pysrc/aiida/transports/plugins/ssh.pysrc/aiida/transports/plugins/ssh_async.pysrc/aiida/transports/transport.pytests/calculations/test_transfer.pytests/cmdline/commands/test_archive_import.pytests/cmdline/commands/test_bug_report.pytests/cmdline/commands/test_data.pytests/cmdline/commands/test_storage.pytests/cmdline/groups/test_dynamic.pytests/cmdline/params/types/test_path.pytests/conftest.pytests/engine/processes/calcjobs/test_calc_job.pytests/engine/test_process_function.pytests/manage/configuration/test_config.pytests/orm/implementation/test_comments.pytests/orm/implementation/test_logs.pytests/orm/nodes/data/test_data.pytests/orm/nodes/data/test_upf.pytests/orm/test_autogroups.pytests/orm/test_entities.pytests/orm/test_fields/fields_aiida.data.core.array.bands.BandsData.ymltests/orm/test_fields/fields_aiida.data.core.array.xy.XyData.ymltests/repository/backend/test_abstract.pytests/repository/conftest.pytests/repository/test_repository.pytests/restapi/conftest.pytests/restapi/test_identifiers.pytests/storage/psql_dos/migrations/django_branch/test_0047_migrate_repository.pytests/storage/psql_dos/migrations/sqlalchemy_branch/test_11_v2_repository.pytests/test_dbimporters.pytests/tools/archive/orm/test_groups.pytests/tools/archive/orm/test_links.pytests/tools/dumping/integration_tests.pytests/transports/test_all_plugins.pytests/utils/archives.pyutils/autogenerate_all_imports.py
🚧 Files skipped from review as they are similar to previous changes (180)
- src/aiida/common/typing.py
- tests/cmdline/commands/test_storage.py
- src/aiida/storage/sqlite_zip/migrations/utils.py
- src/aiida/brokers/rabbitmq/client.py
- src/aiida/tools/query/mapping.py
- src/aiida/schedulers/plugins/pbspro.py
- src/aiida/cmdline/commands/cmd_bug_report.py
- src/aiida/common/folders.py
- src/aiida/cmdline/commands/cmd_rabbitmq.py
- tests/orm/test_autogroups.py
- tests/cmdline/groups/test_dynamic.py
- src/aiida/brokers/zeromq/server.py
- src/aiida/cmdline/params/types/plugin.py
- tests/orm/nodes/data/test_data.py
- src/aiida/cmdline/params/options/main.py
- src/aiida/cmdline/commands/cmd_storage.py
- src/aiida/cmdline/params/types/identifier.py
- src/aiida/orm/nodes/data/enum.py
- src/aiida/tools/pytest_fixtures/entry_points.py
- src/aiida/orm/pydantic.py
- tests/orm/test_entities.py
- tests/cmdline/commands/test_archive_import.py
- src/aiida/schedulers/plugins/sge.py
- src/aiida/storage/psql_dos/migrations/utils/integrity.py
- src/aiida/engine/daemon/worker.py
- src/aiida/cmdline/groups/verdi.py
- src/aiida/manage/profile_access.py
- src/aiida/restapi/translator/base.py
- tests/test_dbimporters.py
- src/aiida/tools/graph/age_rules.py
- tests/cmdline/commands/test_bug_report.py
- src/aiida/tools/_dumping/executors/deletion.py
- src/aiida/orm/utils/managers.py
- docs/source/conf.py
- tests/orm/implementation/test_comments.py
- src/aiida/orm/nodes/process/workflow/workflow.py
- src/aiida/calculations/diff_tutorial/calculations.py
- tests/cmdline/commands/test_data.py
- src/aiida/brokers/zeromq/communicator.py
- src/aiida/tools/archive/common.py
- tests/calculations/test_transfer.py
- src/aiida/engine/processes/futures.py
- src/aiida/cmdline/params/types/code.py
- src/aiida/transports/cli.py
- src/aiida/tools/pytest_fixtures/daemon.py
- src/aiida/orm/utils/calcjob.py
- src/aiida/repository/common.py
- src/aiida/orm/nodes/data/upf.py
- src/aiida/calculations/stash.py
- src/aiida/storage/sqlite_temp/backend.py
- src/aiida/engine/persistence.py
- src/aiida/tools/ipython/ipython_magics.py
- src/aiida/orm/nodes/repository.py
- src/aiida/brokers/broker.py
- tests/repository/conftest.py
- docs/source/topics/processes/include/snippets/functions/typing_none.py
- tests/conftest.py
- src/aiida/tools/visualization/graph.py
- docs/source/topics/processes/include/snippets/functions/typing_union.py
- src/aiida/schedulers/scheduler.py
- src/aiida/storage/sqlite_zip/orm.py
- src/aiida/schedulers/plugins/slurm.py
- src/aiida/brokers/rabbitmq/broker.py
- src/aiida/engine/processes/workchains/awaitable.py
- src/aiida/orm/users.py
- tests/tools/archive/orm/test_groups.py
- tests/orm/implementation/test_logs.py
- src/aiida/orm/nodes/data/array/array.py
- src/aiida/engine/processes/calcjobs/importer.py
- src/aiida/calculations/importers/arithmetic/add.py
- src/aiida/cmdline/commands/cmd_code.py
- src/aiida/orm/implementation/logs.py
- src/aiida/tools/_dumping/executors/collection.py
- src/aiida/engine/launch.py
- src/aiida/orm/nodes/data/array/kpoints.py
- src/aiida/tools/dbimporters/plugins/materialsproject.py
- src/aiida/tools/dbimporters/plugins/icsd.py
- tests/engine/processes/calcjobs/test_calc_job.py
- src/aiida/cmdline/commands/cmd_calcjob.py
- src/aiida/manage/configuration/init.py
- src/aiida/engine/transports.py
- src/aiida/tools/_dumping/executors/process.py
- tests/storage/psql_dos/migrations/django_branch/test_0047_migrate_repository.py
- src/aiida/common/hashing.py
- src/aiida/engine/processes/workchains/workchain.py
- src/aiida/cmdline/utils/echo.py
- src/aiida/tools/graph/deletions.py
- src/aiida/parsers/parser.py
- src/aiida/engine/processes/workchains/utils.py
- src/aiida/orm/implementation/groups.py
- src/aiida/orm/logs.py
- src/aiida/orm/nodes/data/array/bands.py
- src/aiida/orm/nodes/attributes.py
- src/aiida/orm/nodes/data/code/abstract.py
- src/aiida/orm/implementation/authinfos.py
- tests/cmdline/params/types/test_path.py
- src/aiida/calculations/templatereplacer.py
- src/aiida/tools/pytest_fixtures/orm.py
- tests/manage/configuration/test_config.py
- src/aiida/tools/archive/implementations/sqlite_zip/main.py
- src/aiida/orm/nodes/caching.py
- src/aiida/engine/processes/process_spec.py
- src/aiida/storage/psql_dos/migrator.py
- src/aiida/orm/nodes/data/remote/stash/folder.py
- src/aiida/orm/authinfos.py
- .github/system_tests/test_daemon.py
- src/aiida/orm/nodes/data/code/legacy.py
- src/aiida/manage/tests/pytest_fixtures.py
- src/aiida/orm/nodes/data/cif.py
- src/aiida/orm/nodes/data/array/trajectory.py
- src/aiida/orm/nodes/data/remote/stash/compress.py
- src/aiida/storage/psql_dos/backend.py
- src/aiida/orm/nodes/data/remote/base.py
- src/aiida/engine/processes/control.py
- src/aiida/storage/sqlite_zip/utils.py
- tests/repository/backend/test_abstract.py
- src/aiida/manage/configuration/options.py
- src/aiida/tools/_dumping/mapping.py
- src/aiida/transports/plugins/ssh_async.py
- src/aiida/orm/nodes/data/array/xy.py
- tests/repository/test_repository.py
- src/aiida/tools/graph/graph_traversers.py
- src/aiida/cmdline/utils/common.py
- src/aiida/engine/utils.py
- src/aiida/storage/utils.py
- src/aiida/repository/backend/abstract.py
- src/aiida/plugins/entry_point.py
- src/aiida/engine/daemon/client.py
- src/aiida/transports/plugins/local.py
- src/aiida/restapi/translator/nodes/data/init.py
- src/aiida/orm/utils/links.py
- src/aiida/storage/psql_dos/migrations/utils/dblog_update.py
- tests/restapi/test_identifiers.py
- src/aiida/engine/processes/ports.py
- src/aiida/orm/nodes/data/remote/stash/custom.py
- src/aiida/engine/runners.py
- src/aiida/common/progress_reporter.py
- tests/utils/archives.py
- src/aiida/tools/archive/implementations/sqlite_zip/reader.py
- tests/engine/test_process_function.py
- src/aiida/plugins/factories.py
- src/aiida/schedulers/plugins/lsf.py
- src/aiida/orm/nodes/data/dict.py
- src/aiida/manage/configuration/config.py
- src/aiida/storage/sqlite_zip/migrator.py
- src/aiida/orm/nodes/links.py
- src/aiida/engine/processes/functions.py
- src/aiida/engine/processes/calcjobs/tasks.py
- src/aiida/storage/psql_dos/migrations/utils/utils.py
- src/aiida/orm/implementation/entities.py
- src/aiida/repository/repository.py
- src/aiida/tools/archive/create.py
- src/aiida/tools/archive/abstract.py
- src/aiida/tools/groups/paths.py
- src/aiida/orm/entities.py
- src/aiida/orm/nodes/comments.py
- src/aiida/engine/daemon/execmanager.py
- src/aiida/orm/comments.py
- tests/tools/dumping/integration_tests.py
- src/aiida/transports/transport.py
- src/aiida/orm/nodes/process/calculation/calcjob.py
- src/aiida/manage/configuration/profile.py
- src/aiida/cmdline/params/types/path.py
- src/aiida/orm/nodes/node.py
- src/aiida/tools/_dumping/config.py
- src/aiida/storage/sqlite_zip/backend.py
- src/aiida/storage/psql_dos/orm/querybuilder/joiner.py
- src/aiida/orm/implementation/storage_backend.py
- src/aiida/tools/_dumping/tracking.py
- src/aiida/engine/processes/calcjobs/calcjob.py
- src/aiida/transports/plugins/async_backend.py
- src/aiida/tools/archive/implementations/sqlite_zip/writer.py
- src/aiida/transports/plugins/ssh.py
- src/aiida/engine/processes/workchains/restart.py
- src/aiida/orm/groups.py
- src/aiida/storage/psql_dos/orm/querybuilder/main.py
- src/aiida/engine/processes/process.py
- src/aiida/tools/_dumping/utils.py
- src/aiida/tools/archive/imports.py
- src/aiida/orm/querybuilder.py
|
@danielhollas @agoscinski everything green on CI here at the state of 670b900. I'm still doing self-review; will update here tomorrow. |
|
@coderabbitai resolve |
a476194 to
f9e4c26
Compare
✅ Action performedComments resolved. Approval is disabled; enable |
|
Ping @danielhollas @agoscinski. So, I don't expect anybody to manually go through all the hunks of the ~230 changed files here (which I just did), so recording any non-obvious decisions and actions in this document. Scope: every
|
| rule | name | count | kind |
|---|---|---|---|
| UP006 | non-pep585-annotation | 642 | typing |
| UP045 | non-pep604-annotation-optional | 521 | typing |
| UP037 | quoted-annotation | 282 | typing |
| UP035 | deprecated-import | 239 | typing |
| UP007 | non-pep604-annotation-union | 204 | typing |
| UP032 | f-string | 101 | syntax |
| UP034 | extraneous-parentheses | 41 | syntax |
| UP015 | redundant-open-modes | 16 | equivalence |
| UP012 | unnecessary-encode-utf8 | 15 | equivalence |
| UP028 | yield-in-for-loop | 15 | syntax |
| UP041 | timeout-error-alias | 4 | equivalence |
| UP024 | os-error-alias | 2 | equivalence |
| UP031 | printf-string-formatting | 2 | syntax |
| UP008 | super-call-with-parameters | 1 | syntax |
| UP036 | outdated-version-block | 1 | syntax |
| total | 2086 |
Three kinds, and the risk is concentrated in exactly one of them:
- typing (1888, ~91%) — rewrites what
get_origin/get_type_hints/pydantic see at runtime. Both behavior fixes below live here. - syntax (161) — no runtime effect (parens, f-strings,
super(),yield from). - equivalence (37) — touches running code but provably identical on the 3.10 floor:
socket.timeoutisTimeoutError,EnvironmentErrorisOSError,'utf-8'isstr.encode()'s default,'r'isopen()'s default.
Only three rules have unsafe autofixes (applied with --unsafe-fixes): UP028, UP031, UP036. Separately, 193 sites (183 UP035 + 10 UP007) have no autofix at all and were hand-converted.
Before → after for every rule (one real example each, from ruff's own fix preview)
Each example is that rule applied in isolation, so neighbouring constructs can still look un-modernized (UP006's example leaves Union alone because UP007 did not run in that pass). In the sweep the rules compose, so the committed line ends up fully modernized. File references are against upstream/main, i.e. the "before" side.
# UP006 non-pep585-annotation (642) src/aiida/engine/processes/calcjobs/importer.py:21
- def parse_remote_data(remote_data: RemoteData, **kwargs) -> Dict[str, Union[Node, Dict]]:
+ def parse_remote_data(remote_data: RemoteData, **kwargs) -> dict[str, Union[Node, dict]]:
# UP045 non-pep604-annotation-optional (521) docs/.../snippets/functions/typing_none.py:8
- def add_multiply(x: Int, y: Int, z: typing.Optional[Int] = None):
+ def add_multiply(x: Int, y: Int, z: Int | None = None):
# UP037 quoted-annotation (282) src/aiida/brokers/broker.py:42
- def __init__(self, profile: 'Profile') -> None:
+ def __init__(self, profile: Profile) -> None:
# UP035 deprecated-import (239) src/aiida/cmdline/utils/common.py:18
- from typing import TYPE_CHECKING, Any, Literal, Sequence
+ from typing import TYPE_CHECKING, Any, Literal
+ from collections.abc import Sequence
# UP007 non-pep604-annotation-union (204) docs/.../snippets/functions/typing_union.py:8
- def add(x: t.Union[Int, Float], y: t.Union[Int, Float]):
+ def add(x: Int | Float, y: Int | Float):
# UP032 f-string (101) src/aiida/cmdline/commands/cmd_storage.py:98
- confirm_message = 'If you have completed the steps above and want to migrate profile "{}", type {}'.format(
- profile.name, expected_answer
- )
+ confirm_message = f'If you have completed the steps above and want to migrate profile "{profile.name}", type {expected_answer}'
# UP034 extraneous-parentheses (41) src/aiida/manage/tests/pytest_fixtures.py:868
# the genexp is the sole call argument, so its own wrapping parens are redundant;
# note the inner `not (... and ...)` parens are load-bearing and correctly kept
- self.entry_points = EntryPoints((ep for ep in self.entry_points if not (ep.name == name and ep.group == group)))
+ self.entry_points = EntryPoints(ep for ep in self.entry_points if not (ep.name == name and ep.group == group))
# UP015 redundant-open-modes (16) .molecule/default/files/polish/lib/workchain.py:200
- with open(template_file_base, 'r', encoding='utf8') as handle:
+ with open(template_file_base, encoding='utf8') as handle:
# UP012 unnecessary-encode-utf8 (15) src/aiida/cmdline/commands/cmd_bug_report.py:241
- header = f'... (truncated from {size} bytes)\n'.encode('utf-8')
+ header = f'... (truncated from {size} bytes)\n'.encode()
# UP028 yield-in-for-loop (15) [unsafe] src/aiida/storage/psql_dos/orm/extras_mixin.py:88
- for key, value in self.model.extras.items():
- yield key, value
+ yield from self.model.extras.items()
# UP041 timeout-error-alias (4) src/aiida/transports/plugins/ssh.py:1519
- except socket.timeout:
+ except TimeoutError:
# UP024 os-error-alias (2) src/aiida/engine/daemon/execmanager.py:129
- except EnvironmentError as exc:
+ except OSError as exc:
# UP031 printf-string-formatting (2) [unsafe] src/aiida/orm/nodes/data/structure.py:995
- return_string += '%18.10f %18.10f %18.10f\n' % tuple(site.position)
+ return_string += '{:18.10f} {:18.10f} {:18.10f}\n'.format(*tuple(site.position))
# UP008 super-call-with-parameters (1) src/aiida/calculations/diff_tutorial/calculations.py:20
- super(DiffCalculation, cls).define(spec)
+ super().define(spec)
# UP036 outdated-version-block (1) [unsafe] tests/engine/test_process_function.py:731
- if sys.version_info[:2] >= (3, 10):
- expected = (('a', (orm.Str, orm.Int)), ...)
- else:
- expected = (('a', (orm.Data,)), ...)
+ expected = (('a', (orm.Str, orm.Int)), ...)Two of these deserve a note. UP031 rewrites printf-% to .format() rather than to an f-string, because the call uses *-unpacking and there is no way to splat into an f-string; UP032 correctly declines to take it further. UP012 escalates as far as it can prove safe: an ASCII literal becomes a bytes literal ('b'.encode('utf-8') → b'b'), a non-ASCII or f-string literal only loses the redundant argument, and a variable receiver is left alone entirely, since ruff cannot prove it is a str.
Every change traces to a selected rule
The diff is exactly what ruff produces from upstream/main under the configured select (pinned v0.15.21), plus a small set of manual pieces that resolve selected-rule diagnostics ruff cannot autofix: UP007 runtime Union aliases (no autofix in assignment position), the no-autofix half of UP035, and E501 wraps of the f-strings UP032 lengthened. On top of that sit the two behavior fixes and the two regenerated fixtures described below.
Nothing beyond that is included, which is the property that makes a ~230-file diff reviewable: every hunk maps to an enabled rule, so it can be re-derived mechanically rather than read line by line. Concretely, the diff carries no flake8-comprehensions rewrites (set(genexp)/set([..])/dict(genexp) → {..}), because C4 is not in select; and no super() unwrapping inside locally-defined classes, no list→generator unpacking, and no yield from at sites ruff declines to flag. Several of those are behavior-neutral and arguably improvements, but none is reproducible by ruff check as configured, so they stay as upstream/main has them.
One consolidation is included because a UP rule creates it: UP035 rewrites from typing import Mapping as MappingType (in execmanager.py) to collections.abc, which makes it a redundant duplicate of the existing from collections.abc import Mapping, so the now-pointless MappingType alias is dropped in favour of Mapping.
Test fixtures regenerated for the modernized reprs
tests/orm/test_fields.py::test_all_node_fields compares repr() of ORM query fields against committed YAML snapshots (guarded skipif < 3.14). UP006/UP035 changed two dtype reprs the test checks — typing.List[str] | None → list[str] | None (BandsData) and typing.Sequence[str] → collections.abc.Sequence[str] (XyData) — so those two were regenerated. The new reprs are Python-version-stable, so they hold on 3.14 where the test runs.
The same directory also holds 8 fixtures for the process nodes and the Data/ProcessNode/… base classes, which test_all_node_fields never enumerates: its startswith('core.') name filter excludes the entire aiida.node group. Those are deliberately left untouched here. They have been orphaned since the pydantic model rework that introduced that filter (#6990), so most of their staleness is structural drift (QbDictField → QbAttributesField, dtypes becoming the AttributesModel class) rather than repr modernization — refreshing them in this PR would mean rewriting 8 unread files with no test to verify the result. Both the refresh and the filter fix that gives them a test again land in #7448, which also drops the now-obsolete skipif < 3.14 guard, since the modernized reprs are version-stable.
Behavior preservation: two regressions, both from safe autofixes (found + fixed)
Two rewrites silently changed behavior. Both are fixed here, and the interesting part is where they came from: not the three unsafe rules. UP006 and UP045 are marked safe by ruff and applied without --unsafe-fixes. The risk in this sweep is not the 18 unsafe-fix sites, it is the 1888 annotation rewrites — nearly all of them ruff-"safe" — landing on code that introspects annotations at runtime. Neither regression was caught by CI, for different reasons:
-
src/aiida/engine/processes/functions.py—valid_type_map. UP006 rewrote thet.Dict/t.Listkeys todict/list, making them duplicates of the entries already there — and in a dict literal the later key simply wins, so the mapping lost itst.Dict/t.Listentries. Butt.Dict != dictat runtime: they are distinct keys. Process-function input validation for the pre-PEP-585 spelling silently loosened, withOptional[t.Dict]going from requiringorm.Dictto accepting anyData. The keys are restored with an inline# noqa: UP006and a comment saying why collapsing them is the bug. CI was blind because the sweep pyupgraded the test that covered it:test_process_function.pyannotated its caseg: t.Optional[t.Dict], which becameg: dict | None— no longer exercising the lost keys. -
src/aiida/orm/fields.py—extract_root_type. It only unwrappedtyping.Union. The sweep rewrote pydantic model annotations to the PEP 604X | Nonespelling, whoseget_originistypes.UnionType, nottyping.Union, so the union was never unwrapped and the root type came back astypes.UnionType— matching no scalar field class. Measured effect: 207 fields across 40 classes fall back toQbAnyFieldon Python 3.10–3.13, soQueryBuilderwould accept nonsense likeprocess_type > 5. Atypes.UnionTypebranch is added. CI was blind for a structural reason, not because of this PR:test_all_node_fieldsis pinnedskipif < 3.14(#7240), and on 3.14 the bug cannot manifest at all, sincetyping.Union is types.UnionTypethere. The one range where it breaks is exactly the range the test skips.
The fields.py branch therefore still has no guard: it is a no-op on 3.14 and untested below it. #7448 fixes that as a side effect — dropping the obsolete skipif makes the 3.10–3.13 runs fail if the branch is ever removed.
Style: the implicit string concatenation from wrapped f-strings
Applying UP032 to pristine upstream/main creates exactly 61 new E501 violations (measured: 0 before, 61 after) — f-strings that inline their expressions and so run past 120 cols where the .format() call did not. ruff format does not split long strings, so those 61 were wrapped by hand into msg = (f'...' f'...'), i.e. implicit string concatenation. Leaving them that way, deliberately:
-
Nothing in CI objects. This repo does not select the ISC rules, and ruff disables ISC001 whenever its formatter is active anyway; mypy does no stylistic linting.
-
The only tool that objects is basedpyright (locally), whose
reportImplicitStringConcatenationis on by default. That check exists to catch a missing comma in a list (['a' 'b']→['ab']); it cannot fire meaningfully in a single-argument(f'...' f'...')message. -
Extracting to one line doesn't help: these strings are genuinely >120 chars — that is why they wrapped — so a one-liner just trips E501 again. The only clean fix is per-site decomposition into an intermediate variable, e.g.
detail = f'process_state<{calc.process_state}> exit_status<{calc.exit_status}>' print(f'Cached calculation<{calc.pk}> not finished ok: {detail}')
...which is a judgment call on where to split, at each of the 61 sites — contrary to this PR's mechanical, verifiable nature.
-
Decision: leave them here; a focused message-cleanup can be its own PR if we want one later.
Why dict.py imports builtins
UP006 rewrites t.Dict[str, t.Any] to the builtin, but in src/aiida/orm/nodes/data/dict.py it emits builtins.dict[str, t.Any] and adds import builtins. That is correct rather than noise: the Dict node defines a dict property (line 169), which binds the name dict in the class namespace. Annotations on a def line are evaluated as part of the class body (unlike method bodies, which skip class scope), so a bare dict[str, t.Any] written below that property resolves to the property object rather than the type — a TypeError: 'property' object is not subscriptable without from __future__ import annotations, and the wrong type for type checkers with it. Ruff detects the shadow and qualifies the annotation.
Note the positional nuance: value() at line 161 keeps a bare dict because it precedes the property. This is the only module under src/ needing import builtins; QueryBuilder has the same kind of dict method but is unaffected, since all of its dict[...] annotations sit above it. The shadowing itself is a pre-existing wart, written up as #7446.
typing_extensions: can any of it be dropped? (review question from @agoscinski)
Short answer: nothing today, which matches the caveat in the question itself. There are 18 import sites across 5 symbols, the sweep changed none of them, and UP035 flags none of them — correctly, because at requires-python = '>=3.10' every one is gated above the floor:
| symbol | sites | in stdlib typing since |
freed by |
|---|---|---|---|
Self |
10 | 3.11 | dropping py3.10 |
override |
4 | 3.12 | dropping py3.11 |
assert_never |
2 | 3.11 | dropping py3.10 |
NotRequired |
1 | 3.11 | dropping py3.10 |
TypedDict |
2 | 3.8 — but see below | never, by design |
So raising the floor to 3.11 frees 13 sites (Self, assert_never, NotRequired), and 3.12 frees override. UP035 does that rewrite automatically once requires-python moves, since it keys off the target version — verified by running it across targets, where Self/NotRequired become movable at py311 and override at py312.
TypedDict is the exception worth recording, because it looks droppable and is not, for two independent reasons. First, both aiida uses pass closed=True (PEP 728), which is a typing_extensions-only feature absent from stdlib on every version aiida supports — 3.14 included:
class _BasketDict(TypedDict, closed=True, total=True): # tools/graph/age_entities.py
class TraverseGraphOutput(TypedDict, total=False, closed=True): # tools/graph/graph_traversers.py
# checked on both interpreters:
# 3.13.0 and 3.14.3 -> TypeError: _TypedDictMeta.__new__() got an unexpected keyword argument 'closed'
typing.TypedDict(closed=True) -> TypeError
typing_extensions.TypedDict(closed=True) -> OKSecond, ruff never proposes the move anyway, at any target version (py310 through py313 all keep it). UP035 carries an explicit carve-out: "it may be preferable to continue importing members from typing_extensions even after they're added to the Python standard library, as typing_extensions can backport bugfixes and optimizations from later Python versions. This rule thus avoids flagging imports from typing_extensions in such cases." TypedDict is one of those members. So there is no autofix hazard lurking behind a future requires-python bump — the two closed=True sites stay correct on their own.
Minor notes
- UP007 / UP035 manual conversions: ruff offers no autofix for
Union[...]in runtime type-alias assignment position (UP007) or for thetyping.ContextManager→contextlib.AbstractContextManagerimport (UP035); these few were done by hand. - UP037 (
quoted-annotation, 282 sites): safe because the affected modules havefrom __future__ import annotations, so annotations are lazy strings never evaluated at runtime — the quotes were pure redundancy, existing only to defer aTYPE_CHECKING-only name, which the future import already does module-wide. Verified every__init__(profile)site still imports cleanly, i.e. the sweep created noNameErrorin a module that happened to lack the future import. The quotes are correctly kept where they do real work: in runtime type-alias assignments such asEntityClsType = type[Union[entities.Entity, 'Process']], which PEP 563 does not defer, so unquoting would be an import-timeNameError. - UP008 (
super-call-with-parameters):super(DiffCalculation, cls).define(spec)→super().define(spec). Inside a method, zero-argumentsuper()is exactly equivalent — the compiler injects the__class__cell and binds the first positional argument automatically. Behavior-identical. - UP028 (
yield-in-for-loop):for task in it: yield task→yield from it. Equivalent for plain iteration (andyield fromadditionally forwards.send()/.throw(), a strict superset). - UP036 (
outdated-version-block, one of the three unsafe fixes): the removedelsebranch intests/engine/test_process_function.pywas unreachable on every supported Python, since the floor is 3.10, so only dead code goes. One knock-on: that branch was the sole consumer of the test'scaplogfixture, now an unused argument. No selected rule flags it (ARG001would, butARGis not inselect), so it is left alone rather than hand-removed.
Apply the changes ruff's `UP` (pyupgrade) rule set produces across the codebase, ahead of enabling the rules themselves in the following commit. Splitting it this way keeps the mechanical churn in a commit that can be listed in `.git-blame-ignore-revs`, while the decision to enable `UP` stays attributable in `pyproject.toml`. 2086 violations across 15 rules, dominated by typing modernization (UP006 642, UP045 521, UP037 282, UP035 239, UP007 204, about 91%): `typing.Dict`/`List`/... -> builtins (PEP 585), `Optional[X]` -> `X | None`, `Union[X, Y]` -> `X | Y`, quoted-annotation removal, and deprecated `typing` imports -> `collections.abc`. The rest are `.format()` -> f-strings plus the smaller UP008/UP012/UP015/UP024/ UP028/UP031/UP034/UP036/UP041. Hand-convert the 193 sites ruff cannot autofix (183 UP035, 10 UP007: runtime `Union` aliases and deprecated-import rewrites) and wrap the 61 f-strings UP032 pushed past the 120-col limit; message text is preserved byte-for-byte. Two annotation rewrites are handled manually to preserve behavior: - `functions.py`: keep the distinct `t.Dict`/`t.List` keys in `valid_type_map` -- they are `!=` their builtins at runtime, so collapsing them would loosen process-function input validation for the pre-PEP-585 spelling. - `fields.py`: `extract_root_type` unwraps both `typing.Union` and the PEP 604 `types.UnionType`, so the rewrites do not degrade 207 ORM query fields across 40 classes to `QbAnyField` on Python 3.10-3.13. Consolidate the two now-identical `Mapping` imports in `execmanager.py` that UP035 leaves behind, and regenerate the two `test_all_node_fields` snapshots whose dtype reprs changed.
f9e4c26 to
23dfc06
Compare
Add `UP` to the ruff lint `select` list. The codebase was modernized in the preceding commit, so turning the rules on here produces no further code changes: `ruff check` is clean on both sides of this commit. Keeping the switch in its own commit means the preceding one, which touches ~230 files purely to rewrite typing and formatting, can be listed in `.git-blame-ignore-revs` without also hiding the decision to enable `UP` from `git blame pyproject.toml`. The two `# noqa: UP006` in `valid_type_map` land here rather than with the code they annotate, because they suppress a rule that does not exist until this commit: with `UP` unselected they would be reported as unused directives (RUF100). They mark the `t.Dict`/`t.List` keys that must stay distinct from their builtins.
|
github is overloaded, cannot approve, just bypass |
Apply the changes ruff's `UP` (pyupgrade) rule set produces across the codebase, ahead of enabling the rules themselves in the following commit. Splitting it this way keeps the mechanical churn in a commit that can be listed in `.git-blame-ignore-revs`, while the decision to enable `UP` stays attributable in `pyproject.toml`. 2086 violations across 15 rules, dominated by typing modernization (UP006 642, UP045 521, UP037 282, UP035 239, UP007 204, about 91%): `typing.Dict`/`List`/... -> builtins (PEP 585), `Optional[X]` -> `X | None`, `Union[X, Y]` -> `X | Y`, quoted-annotation removal, and deprecated `typing` imports -> `collections.abc`. The rest are `.format()` -> f-strings plus the smaller UP008/UP012/UP015/UP024/ UP028/UP031/UP034/UP036/UP041. Hand-convert the 193 sites ruff cannot autofix (183 UP035, 10 UP007: runtime `Union` aliases and deprecated-import rewrites) and wrap the 61 f-strings UP032 pushed past the 120-col limit; message text is preserved byte-for-byte. Two annotation rewrites are handled manually to preserve behavior: - `functions.py`: keep the distinct `t.Dict`/`t.List` keys in `valid_type_map` -- they are `!=` their builtins at runtime, so collapsing them would loosen process-function input validation for the pre-PEP-585 spelling. - `fields.py`: `extract_root_type` unwraps both `typing.Union` and the PEP 604 `types.UnionType`, so the rewrites do not degrade 207 ORM query fields across 40 classes to `QbAnyField` on Python 3.10-3.13. Consolidate the two now-identical `Mapping` imports in `execmanager.py` that UP035 leaves behind, and regenerate the two `test_all_node_fields` snapshots whose dtype reprs changed.
The `node_and_data_entry_points` fixture filtered entry points with
`startswith('core.')`, but every `aiida.node` name starts with `data` or
`process`, never `core.`, so the whole group was silently excluded.
`test_all_node_fields` therefore only ever checked the 32 `aiida.data`
classes, leaving the process nodes and the `Data`/`ProcessNode`/
`CalculationNode`/`WorkflowNode` bases with no field-schema coverage.
Drop the filter so they are checked again.
Refresh the eight reference fixtures the filter had orphaned. They had
drifted since the pydantic model rework that introduced the filter and
still carried the pre-PEP-585 dtype reprs (`typing.List[str]` ->
`list[str]`, `typing.Sequence` -> `collections.abc.Sequence`); only now
do they gain a test that verifies them.
Also drop the `skipif < 3.14` guard. It existed because `repr()` of
`typing` generics is not stable across versions, but the annotations are
now PEP 585/604 throughout (aiidateam#7254), so every field repr is
version-stable and the fixtures hold on 3.10-3.14. That matters beyond
coverage: it restores the only CI check of `extract_root_type`'s
`types.UnionType` branch, which is a no-op on 3.14 (where `t.Union is
UnionType`) but load-bearing below it, so removing that branch now fails
the suite instead of silently degrading 187 query fields to
`QbAnyField`.
The `node_and_data_entry_points` fixture filtered entry points with
`startswith('core.')`, but every `aiida.node` name starts with `data` or
`process`, never `core.`, so the whole group was silently excluded.
`test_all_node_fields` therefore only ever checked the 32 `aiida.data`
classes, leaving the process nodes and the `Data`/`ProcessNode`/
`CalculationNode`/`WorkflowNode` bases with no field-schema coverage.
Drop the filter so they are checked again.
Refresh the eight reference fixtures the filter had orphaned. They had
drifted since the pydantic model rework that introduced the filter and
still carried the pre-PEP-585 dtype reprs (`typing.List[str]` ->
`list[str]`, `typing.Sequence` -> `collections.abc.Sequence`); only now
do they gain a test that verifies them.
Also drop the `skipif < 3.14` guard. It existed because `repr()` of
`typing` generics is not stable across versions, but the annotations are
now PEP 585/604 throughout (#7254), so every field repr is
version-stable and the fixtures hold on 3.10-3.14. That matters beyond
coverage: it restores the only CI check of `extract_root_type`'s
`types.UnionType` branch, which is a no-op on 3.14 (where `t.Union is
UnionType`) but load-bearing below it, so removing that branch now fails
the suite instead of silently degrading 187 query fields to
`QbAnyField`.
Apply the changes ruff's `UP` (pyupgrade) rule set produces across the codebase, ahead of enabling the rules themselves in the following commit. Splitting it this way keeps the mechanical churn in a commit that can be listed in `.git-blame-ignore-revs`, while the decision to enable `UP` stays attributable in `pyproject.toml`. 2086 violations across 15 rules, dominated by typing modernization (UP006 642, UP045 521, UP037 282, UP035 239, UP007 204, about 91%): `typing.Dict`/`List`/... -> builtins (PEP 585), `Optional[X]` -> `X | None`, `Union[X, Y]` -> `X | Y`, quoted-annotation removal, and deprecated `typing` imports -> `collections.abc`. The rest are `.format()` -> f-strings plus the smaller UP008/UP012/UP015/UP024/ UP028/UP031/UP034/UP036/UP041. Hand-convert the 193 sites ruff cannot autofix (183 UP035, 10 UP007: runtime `Union` aliases and deprecated-import rewrites) and wrap the 61 f-strings UP032 pushed past the 120-col limit; message text is preserved byte-for-byte. Two annotation rewrites are handled manually to preserve behavior: - `functions.py`: keep the distinct `t.Dict`/`t.List` keys in `valid_type_map` -- they are `!=` their builtins at runtime, so collapsing them would loosen process-function input validation for the pre-PEP-585 spelling. - `fields.py`: `extract_root_type` unwraps both `typing.Union` and the PEP 604 `types.UnionType`, so the rewrites do not degrade 207 ORM query fields across 40 classes to `QbAnyField` on Python 3.10-3.13. Consolidate the two now-identical `Mapping` imports in `execmanager.py` that UP035 leaves behind, and regenerate the two `test_all_node_fields` snapshots whose dtype reprs changed.
Add `UP` to the ruff lint `select` list. The codebase was modernized in the preceding commit, so turning the rules on here produces no further code changes: `ruff check` is clean on both sides of this commit. Keeping the switch in its own commit means the preceding one, which touches ~230 files purely to rewrite typing and formatting, can be listed in `.git-blame-ignore-revs` without also hiding the decision to enable `UP` from `git blame pyproject.toml`. The two `# noqa: UP006` in `valid_type_map` land here rather than with the code they annotate, because they suppress a rule that does not exist until this commit: with `UP` unselected they would be reported as unused directives (RUF100). They mark the `t.Dict`/`t.List` keys that must stay distinct from their builtins.
The `node_and_data_entry_points` fixture filtered entry points with
`startswith('core.')`, but every `aiida.node` name starts with `data` or
`process`, never `core.`, so the whole group was silently excluded.
`test_all_node_fields` therefore only ever checked the 32 `aiida.data`
classes, leaving the process nodes and the `Data`/`ProcessNode`/
`CalculationNode`/`WorkflowNode` bases with no field-schema coverage.
Drop the filter so they are checked again.
Refresh the eight reference fixtures the filter had orphaned. They had
drifted since the pydantic model rework that introduced the filter and
still carried the pre-PEP-585 dtype reprs (`typing.List[str]` ->
`list[str]`, `typing.Sequence` -> `collections.abc.Sequence`); only now
do they gain a test that verifies them.
Also drop the `skipif < 3.14` guard. It existed because `repr()` of
`typing` generics is not stable across versions, but the annotations are
now PEP 585/604 throughout (aiidateam#7254), so every field repr is
version-stable and the fixtures hold on 3.10-3.14. That matters beyond
coverage: it restores the only CI check of `extract_root_type`'s
`types.UnionType` branch, which is a no-op on 3.14 (where `t.Union is
UnionType`) but load-bearing below it, so removing that branch now fails
the suite instead of silently degrading 187 query fields to
`QbAnyField`.
Apply the changes ruff's `UP` (pyupgrade) rule set produces across the codebase, ahead of enabling the rules themselves in the following commit. Splitting it this way keeps the mechanical churn in a commit that can be listed in `.git-blame-ignore-revs`, while the decision to enable `UP` stays attributable in `pyproject.toml`. 2086 violations across 15 rules, dominated by typing modernization (UP006 642, UP045 521, UP037 282, UP035 239, UP007 204, about 91%): `typing.Dict`/`List`/... -> builtins (PEP 585), `Optional[X]` -> `X | None`, `Union[X, Y]` -> `X | Y`, quoted-annotation removal, and deprecated `typing` imports -> `collections.abc`. The rest are `.format()` -> f-strings plus the smaller UP008/UP012/UP015/UP024/ UP028/UP031/UP034/UP036/UP041. Hand-convert the 193 sites ruff cannot autofix (183 UP035, 10 UP007: runtime `Union` aliases and deprecated-import rewrites) and wrap the 61 f-strings UP032 pushed past the 120-col limit; message text is preserved byte-for-byte. Two annotation rewrites are handled manually to preserve behavior: - `functions.py`: keep the distinct `t.Dict`/`t.List` keys in `valid_type_map` -- they are `!=` their builtins at runtime, so collapsing them would loosen process-function input validation for the pre-PEP-585 spelling. - `fields.py`: `extract_root_type` unwraps both `typing.Union` and the PEP 604 `types.UnionType`, so the rewrites do not degrade 207 ORM query fields across 40 classes to `QbAnyField` on Python 3.10-3.13. Consolidate the two now-identical `Mapping` imports in `execmanager.py` that UP035 leaves behind, and regenerate the two `test_all_node_fields` snapshots whose dtype reprs changed.
Add `UP` to the ruff lint `select` list. The codebase was modernized in the preceding commit, so turning the rules on here produces no further code changes: `ruff check` is clean on both sides of this commit. Keeping the switch in its own commit means the preceding one, which touches ~230 files purely to rewrite typing and formatting, can be listed in `.git-blame-ignore-revs` without also hiding the decision to enable `UP` from `git blame pyproject.toml`. The two `# noqa: UP006` in `valid_type_map` land here rather than with the code they annotate, because they suppress a rule that does not exist until this commit: with `UP` unselected they would be reported as unused directives (RUF100). They mark the `t.Dict`/`t.List` keys that must stay distinct from their builtins.
The `node_and_data_entry_points` fixture filtered entry points with
`startswith('core.')`, but every `aiida.node` name starts with `data` or
`process`, never `core.`, so the whole group was silently excluded.
`test_all_node_fields` therefore only ever checked the 32 `aiida.data`
classes, leaving the process nodes and the `Data`/`ProcessNode`/
`CalculationNode`/`WorkflowNode` bases with no field-schema coverage.
Drop the filter so they are checked again.
Refresh the eight reference fixtures the filter had orphaned. They had
drifted since the pydantic model rework that introduced the filter and
still carried the pre-PEP-585 dtype reprs (`typing.List[str]` ->
`list[str]`, `typing.Sequence` -> `collections.abc.Sequence`); only now
do they gain a test that verifies them.
Also drop the `skipif < 3.14` guard. It existed because `repr()` of
`typing` generics is not stable across versions, but the annotations are
now PEP 585/604 throughout (aiidateam#7254), so every field repr is
version-stable and the fixtures hold on 3.10-3.14. That matters beyond
coverage: it restores the only CI check of `extract_root_type`'s
`types.UnionType` branch, which is a no-op on 3.14 (where `t.Union is
UnionType`) but load-bearing below it, so removing that branch now fails
the suite instead of silently degrading 187 query fields to
`QbAnyField`.
See here: #7254 (comment)