Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7629 +/- ##
==========================================
+ Coverage 81.69% 81.80% +0.12%
==========================================
Files 621 621
Lines 51529 51780 +251
==========================================
+ Hits 42092 42355 +263
+ Misses 9437 9425 -12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
GeigerJ2
force-pushed
the
v3/7615/callable-transport
branch
from
September 11, 2026 20:35
9930c83 to
55b0ee8
Compare
GeigerJ2
added a commit
to GeigerJ2/aiida-core
that referenced
this pull request
Sep 11, 2026
`is_importable` answers about the interpreter it runs in, which is the wrong one whenever the object is read back somewhere else. A module in a directory added to `sys.path` imports here and not in a process that started before it was added, and a name that finds a *different* file there is worse than one that finds nothing, since it runs other code without saying so. `resolves_in` asks the question of another interpreter's paths instead, by resolving the name against them and comparing the file that would be found to the file the object came from. Where the name does not reach, the object has to travel, and so do the modules the reader lacks. `modules_missing_from` lists them: every module loaded here that an interpreter answering for the reader could not import. Which of them a given callable needs is never asked, because `cloudpickle` writes only what the object it is handed reaches, so naming a module that object never touches produces the same bytes. Deriving the set from the callable would buy nothing for that cost, and would miss whatever it reaches at runtime, through `importlib.import_module` or an object built from data. A module the reader already has stays a reference, because carrying it would bloat the payload and pin the reader to this interpreter's copy. `dumps` takes that set. The registry it registers them in is process global, so it only undoes the registrations it made itself. `sys.modules` holds more than modules: a package may leave a lazy stand in there, a failed import can leave `None` behind, and an alias puts a module under a second name that registering by value would not use. Only an entry that is a module and knows itself by the name it is filed under counts. `source_of` grows a fallback for the same reason a class defined in a notebook has no source: `inspect` reaches a class through the file of the module defining it, and a kernel's `__main__` has none. Its own methods carry the cell they were compiled from, and the class statement is in that cell.
GeigerJ2
added a commit
to GeigerJ2/aiida-core
that referenced
this pull request
Sep 11, 2026
A daemon worker imports from the `sys.path` of the process that started the daemon, replicated to it as `PYTHONPATH` and frozen there. So a name persisted for a worker to resolve later is recoverable only if that path finds it, and the interpreter doing the persisting can extend its own path long afterwards. What one can import is not what the other can. The env info file the daemon already writes at startup gains those paths, alongside the package versions and the interpreter it records. `get_daemon_import_paths` reads them back, cached on the file's modification time, so a restart invalidates the entry instead of serving a path the daemon no longer has. Assembling the path of that file walks the whole configuration, which is too slow to repeat per lookup, so it is cached per profile as well. The field is optional, leaving a file written before this readable.
GeigerJ2
added a commit
to GeigerJ2/aiida-core
that referenced
this pull request
Sep 11, 2026
PyYAML writes a callable as a reference to its `__module__` and `__name__`, so a lambda, a closure, or anything defined in `__main__` went into a checkpoint under a name that resolves to nothing. `__qualname__` is dropped along the way, so `make_parser.<locals>.parse` was written as `parse` and would have loaded a module level function of that name had one existed. A `functools.partial` could not be represented at all. A name can also resolve here and not in the worker that reads the checkpoint back, since that worker imports from the path the daemon froze at startup. A directory added to a submitting shell afterwards is importable only in that shell. A name that would find a different file there is worse than one that finds nothing, because it runs other code without saying so. Both are the same question: whether the reader recovers this object from its name. Where it cannot, the object is written under `!aiida_callable` as bytes, together with the modules the reader lacks. Modules the reader has stay references, so a payload never carries an installed dependency nor pins the version of one. An answer claiming the reader lacks most of what is loaded describes a broken reader rather than a real one, and acting on it would mean carrying the interpreter's own machinery: registering `cloudpickle` by value recurses until the stack is gone. Past that ceiling the names are kept and nothing is carried, which is how this behaved before anything was known about the reader, and the log says so. The class of the process is the same question asked once more, and it is asked of the identifier rather than of the class, because a process built from a function is identified by that function and not by its own dynamically built class. A class the worker cannot import travels in the checkpoint beside its name, which lets a workchain defined in a notebook run on the daemon at all. One that cannot be serialized, such as a class defined inside a function that closes over a node, keeps the name it had and fails exactly where it always did. The question is answered in one place, next to the checkpoint it is about, since the serializer and the class identity both ask it.
GeigerJ2
force-pushed
the
v3/7615/callable-transport
branch
from
September 11, 2026 21:12
55b0ee8 to
af59a1c
Compare
GeigerJ2
added a commit
to GeigerJ2/aiida-core
that referenced
this pull request
Sep 11, 2026
`build_process_type` records the module a class was defined in whenever no entry point registers it, and for a notebook cell or a script that module is `__main__`. That names the entry point of whichever interpreter is asking, so the string identifies a different module everywhere else, permanently, in every database that holds the node. Encoding identity into that string does not help. Subclass queries take the part before the last two dots, so every `__main__` class already collapses to the same prefix whatever is appended, and appending anything at all breaks the entry point form the string otherwise has. So the string is left alone and the class is recorded beside it: a fingerprint, which separates two classes that share a name, and the source, which is the only thing that says what actually ran once the checkpoint carrying the class is gone. `ProcessNode.class_source` reads it back. `process_class` now says that the class was defined where no name reaches it, rather than raising an import error about `__main__`, a module that does exist and does not hold what is being looked for.
GeigerJ2
added a commit
to GeigerJ2/aiida-core
that referenced
this pull request
Sep 11, 2026
A `Parser` reads the output spec, the exit codes and the retrieved link label off `self.node.process_class`, which resolves the name the class was recorded under. A class defined where no name reaches it, such as in a notebook, has no such name, so parsing raised even though the class itself had travelled in the checkpoint and the calculation had run. The running process knows its own class and now says so. The node stays the fallback, which is what parsing a stored node afterwards uses, and what every parser that resolves fine today keeps doing. That leaves a calculation whose class no name identifies runnable, and its outputs parsed, while `parse_from_node` on the stored node still cannot: there is no process to ask by then, and the checkpoint that carried the class is deleted when the node seals.
GeigerJ2
added a commit
to GeigerJ2/aiida-core
that referenced
this pull request
Sep 11, 2026
The unit tests pin each decision on its own: whether a name resolves for the reader, which modules have to travel, what the record holds. None of them runs a worker, and the thing being claimed is that a process the daemon cannot import by name runs there anyway. These submit to a real daemon and wait. A workchain in a module only the submitting interpreter can import, a workchain and a process function reported as `__main__` the way a notebook cell reports them, a shell job whose parser calls into such a module, and a calculation job, which needs the parser to ask the process for its class rather than the node. An installed plugin is in there too, asserting the opposite: that it carries no payload, which is what keeps an ordinary checkpoint the size it was. The module the daemon cannot import is built after the daemon fixture has started it, since a daemon inherits the `sys.path` of whatever started it and would otherwise be able to import it after all. Each fixture asserts that it really is out of reach before the test runs, so a test that stops proving anything fails rather than passing quietly.
GeigerJ2
force-pushed
the
v3/7615/callable-transport
branch
from
September 11, 2026 21:27
af59a1c to
1036d49
Compare
A pickled callable stored as a node puts its bytes in the repository, the graph and every archive, permanently, and reading it means running the code inside it. `CallableData` records what a callable is instead: its source text, where that source came from, and one of three ways to identify it. An entry point, if a plugin registers it, since that survives the callable being moved. Otherwise the module and qualified name that import it. Otherwise a fingerprint of its serialized form. The last two are kept apart on purpose. Closures built by the same factory share a name, a source text and a source location, so only what they capture separates them. Conversely, fingerprinting an importable callable would tie the node hash to the installed pickler version, where its name already identifies it exactly. A callable that is neither importable nor serializable is refused, since without a fingerprint the record could not tell it from any other closure. `aiida.common.callables` holds the primitives, low enough that the checkpoint serializer can use them without importing the ORM. It serializes with `cloudpickle`, which covers closures, lambdas, `functools.partial` and callable objects, where `dill` writes a reference for anything it can name. `dill` cannot serialize by value at any setting: `byref` and `recurse` make no difference to how it writes an importable function.
PyYAML writes a callable as a reference to its `__module__` and `__name__`, so a lambda, a closure, or anything defined in `__main__` went into a checkpoint under a name that resolves to nothing. `__qualname__` is dropped along the way, so `make_parser.<locals>.parse` was written as `parse` and would have loaded a module level function of that name had one existed. A `functools.partial` could not be represented at all. Nothing reached this. `Process.__init__` serializes inputs into nodes before they become the raw inputs, and `ProcessBuilder` does the same on assignment, so no callable ever entered a checkpoint. It was a latent defect rather than a failure anyone could hit, and the next commit, which persists a callable as a member of the process, is the first thing to depend on it. Such callables now go under `!aiida_callable` as bytes, while anything that can be imported keeps the name reference it had.
The `parser` input stored the callable as a pickled node, which the parser unpickled in order to call it. It is now recorded in a `CallableData`, and the callable itself is persisted with the process, so a worker still has it after rebuilding the process from its checkpoint to parse a job that finished meanwhile. `ProcessBuilder` runs the port serializer on assignment, so the raw callable is already gone from the raw inputs by the time the process exists. The record therefore carries it across: a `CallableData` keeps a reference to the callable it recorded, which `on_create` takes off it. A `Parser` is constructed from the node alone, so `CalcJob` gains a `_get_parse_kwargs` hook for process state that a parser needs. An entry point string is recorded the same way, so the port takes one node type and no consumer has to branch on which it was handed. Validation reads the parameters the record stored, rather than loading the parser to inspect its signature, and rejects one whose arguments cannot be supplied positionally, which is how the hook is called. A parser that cannot be imported is no longer re-runnable from a stored node. The archive shows what parsed the job and its source, and carries no code that runs when the node is read.
`@pytest.mark.usefixtures` has no effect when applied to a fixture, so the marker on `setup_codes` never ran `aiida_profile_clean` and the fixture kept starting from whatever the previous test had left in the profile. Requesting the fixture as an argument is what the rest of the suite does. The symptom is `UNIQUE constraint failed: db_dbcomputer.label` at setup of the tests that use it, once an earlier test has already created a computer with that label.
Nothing produces either of them any more. A callable handed to `ShellJob`, as an object or as an entry point string, is recorded in a `CallableData`, which stores a description of it rather than the callable itself. Removing them needs no migration. `load_node_class` falls back to the base `Data` class for any `aiida.data` entry point it cannot find, so a node that `aiida-shell` wrote keeps its type string, still loads, and still hands back its attributes and its repository contents. What such a node loses is `PickledData.load()`, the method that ran the code in the pickle, which is the reason the class is going in the first place. `dill` was there only for `PickledData` and goes with it. Callables are serialized with `cloudpickle`.
`is_importable` answers about the interpreter it runs in, which is the wrong one whenever the object is read back somewhere else. A module in a directory added to `sys.path` imports here and not in a process that started before it was added, and a name that finds a *different* file there is worse than one that finds nothing, since it runs other code without saying so. `resolves_in` asks the question of another interpreter's paths instead, by resolving the name against them and comparing the file that would be found to the file the object came from. Where the name does not reach, the object has to travel, and so do the modules the reader lacks. `modules_missing_from` lists them: every module loaded here that an interpreter answering for the reader could not import. Which of them a given callable needs is never asked, because `cloudpickle` writes only what the object it is handed reaches, so naming a module that object never touches produces the same bytes. Deriving the set from the callable would buy nothing for that cost, and would miss whatever it reaches at runtime, through `importlib.import_module` or an object built from data. A module the reader already has stays a reference, because carrying it would bloat the payload and pin the reader to this interpreter's copy. `dumps` takes that set. The registry it registers them in is process global, so it only undoes the registrations it made itself. `sys.modules` holds more than modules: a package may leave a lazy stand in there, a failed import can leave `None` behind, and an alias puts a module under a second name that registering by value would not use. Only an entry that is a module and knows itself by the name it is filed under counts. `source_of` grows a fallback for the same reason a class defined in a notebook has no source: `inspect` reaches a class through the file of the module defining it, and a kernel's `__main__` has none. Its own methods carry the cell they were compiled from, and the class statement is in that cell.
A daemon worker imports from the `sys.path` of the process that started the daemon, replicated to it as `PYTHONPATH` and frozen there. So a name persisted for a worker to resolve later is recoverable only if that path finds it, and the interpreter doing the persisting can extend its own path long afterwards. What one can import is not what the other can. The env info file the daemon already writes at startup gains those paths, alongside the package versions and the interpreter it records. `get_daemon_import_paths` reads them back, cached on the file's modification time, so a restart invalidates the entry instead of serving a path the daemon no longer has. Assembling the path of that file walks the whole configuration, which is too slow to repeat per lookup, so it is cached per profile as well. The field is optional, leaving a file written before this readable.
PyYAML writes a callable as a reference to its `__module__` and `__name__`, so a lambda, a closure, or anything defined in `__main__` went into a checkpoint under a name that resolves to nothing. `__qualname__` is dropped along the way, so `make_parser.<locals>.parse` was written as `parse` and would have loaded a module level function of that name had one existed. A `functools.partial` could not be represented at all. A name can also resolve here and not in the worker that reads the checkpoint back, since that worker imports from the path the daemon froze at startup. A directory added to a submitting shell afterwards is importable only in that shell. A name that would find a different file there is worse than one that finds nothing, because it runs other code without saying so. Both are the same question: whether the reader recovers this object from its name. Where it cannot, the object is written under `!aiida_callable` as bytes, together with the modules the reader lacks. Modules the reader has stay references, so a payload never carries an installed dependency nor pins the version of one. An answer claiming the reader lacks most of what is loaded describes a broken reader rather than a real one, and acting on it would mean carrying the interpreter's own machinery: registering `cloudpickle` by value recurses until the stack is gone. Past that ceiling the names are kept and nothing is carried, which is how this behaved before anything was known about the reader, and the log says so. The class of the process is the same question asked once more, and it is asked of the identifier rather than of the class, because a process built from a function is identified by that function and not by its own dynamically built class. A class the worker cannot import travels in the checkpoint beside its name, which lets a workchain defined in a notebook run on the daemon at all. One that cannot be serialized, such as a class defined inside a function that closes over a node, keeps the name it had and fails exactly where it always did. The question is answered in one place, next to the checkpoint it is about, since the serializer and the class identity both ask it.
`build_process_type` records the module a class was defined in whenever no entry point registers it, and for a notebook cell or a script that module is `__main__`. That names the entry point of whichever interpreter is asking, so the string identifies a different module everywhere else, permanently, in every database that holds the node. Encoding identity into that string does not help. Subclass queries take the part before the last two dots, so every `__main__` class already collapses to the same prefix whatever is appended, and appending anything at all breaks the entry point form the string otherwise has. So the string is left alone and the class is recorded beside it: a fingerprint, which separates two classes that share a name, and the source, which is the only thing that says what actually ran once the checkpoint carrying the class is gone. `ProcessNode.class_source` reads it back. `process_class` now says that the class was defined where no name reaches it, rather than raising an import error about `__main__`, a module that does exist and does not hold what is being looked for.
A `Parser` reads the output spec, the exit codes and the retrieved link label off `self.node.process_class`, which resolves the name the class was recorded under. A class defined where no name reaches it, such as in a notebook, has no such name, so parsing raised even though the class itself had travelled in the checkpoint and the calculation had run. The running process knows its own class and now says so. The node stays the fallback, which is what parsing a stored node afterwards uses, and what every parser that resolves fine today keeps doing. That leaves a calculation whose class no name identifies runnable, and its outputs parsed, while `parse_from_node` on the stored node still cannot: there is no process to ask by then, and the checkpoint that carried the class is deleted when the node seals.
The unit tests pin each decision on its own: whether a name resolves for the reader, which modules have to travel, what the record holds. None of them runs a worker, and the thing being claimed is that a process the daemon cannot import by name runs there anyway. These submit to a real daemon and wait. A workchain in a module only the submitting interpreter can import, a workchain and a process function reported as `__main__` the way a notebook cell reports them, a shell job whose parser calls into such a module, and a calculation job, which needs the parser to ask the process for its class rather than the node. An installed plugin is in there too, asserting the opposite: that it carries no payload, which is what keeps an ordinary checkpoint the size it was. The module the daemon cannot import is built after the daemon fixture has started it, since a daemon inherits the `sys.path` of whatever started it and would otherwise be able to import it after all. Each fixture asserts that it really is out of reach before the test runs, so a test that stops proving anything fails rather than passing quietly.
GeigerJ2
added
the
pr/blocked
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
Currently blocked on the changes in #7617. Might consolidate this PR and #7617 again, not sure yet. Split seemed to make sense, conceptually. Don't review here yet, before #7617 is merged, and this PR rebased on it.
A
WorkChain, acalcfunctionor aCalcJobwritten in a notebook cell can now be submitted to the daemon. Before, it was created and then excepted in the worker withImportError: object 'NotebookWorkChain' from identifier '__main__:NotebookWorkChain' could not be loaded, because a cell's__main__is the kernel and the worker's is the daemon. Builds on #7617, which records what a callable is.Underneath is one question, asked wherever a name has to survive into another interpreter: does that interpreter resolve this name to the same object? A daemon worker imports from the
sys.paththe daemon froze at startup, so a name can resolve in a submitting shell and not in the worker, and one that finds a different file there is worse than one that finds nothing. Where the answer is no, the object travels in the checkpoint together with the modules it reaches that the worker lacks. Modules the worker already has stay references, so a payload never carries an installed dependency nor pins the version of one.Asking that of the process class too is what makes notebooks work: a
WorkChain, acalcfunctionor aCalcJobdefined in a cell now runs on the daemon, where before it excepted in the worker withImportError: object 'NotebookWorkChain' ... could not be loaded. A class that cannot be serialized, such as one defined inside a function that closes over a node, keeps the name it had and behaves as before.process_typestill records__main__.NotebookWorkChain, since nothing encodable in that string helps: subclass queries takersplit('.', 2)[0], so every__main__class collapses to the same prefix whatever you append. The class source and a fingerprint go on the node instead, readable throughProcessNode.class_sourceonce the checkpoint carrying the class is gone.tests/engine/test_callables_through_the_daemon.pyruns each case through a real worker, including one the daemon genuinely cannot import.