[minor] 0.10.0 #413
Merged
[minor] 0.10.0 #413
Conversation
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
With the introduction of a pickle backend, this test wound up failing. I haven't been able to work out exactly why, but since pickle is much faster my guess is that something in the test parallelization gets in a race condition with deleting a saved workflow from another test. Simply renaming it here solved the issue, which is an innocuous change.
Ok, the reason workflow storage tests were failing after introducing pickle was that the original version of the test had the wrong order of the try/finally scope and the for-loop scope. I got away with it earlier because of interplay between the outer-most member of the loop and the default storage backend. Actually fixing the problem is as simple as ensuring the "finally delete" clause happens after _each_ loop step. This does that and reverts the renaming of the workflow.
You're only supposed to use it as a decorator to start with, so the kwarg was senseless
This is necessary (but not sufficient) to get `as_dataclass_node` decorated classes to pickle. The field name is a bit off compared to `Function` and `Macro`, as now we decorate a class definition instead of a function definition, but it's close.
Now that it's pickling as well as anything else
When the pickle backend fails
Regardless of what the specified storage backend was.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
|
* Remove all storage backends except for pickle * Remove pyiron_contrib dependence and references * Remove h5io_browser dependence * Remove no-bidict workaround It was only necessary for h5io since that can't handle bidict's custom reconstructor * Remove channel recursion workaround It was only necessary for h5io since that can't handle recursion. We keep the purging of connections, because those might actually extend the data scope by reaching non-owner nodes * [minor] Remove node job (#415) * Remove node job * Remove pyiron_base and h5io dependencies
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
Pull Request Test Coverage Report for Build 10517318730Details
💛 - Coveralls |
* Remove all storage backends except for pickle * Remove pyiron_contrib dependence and references * Remove h5io_browser dependence * Remove no-bidict workaround It was only necessary for h5io since that can't handle bidict's custom reconstructor * Remove channel recursion workaround It was only necessary for h5io since that can't handle recursion. We keep the purging of connections, because those might actually extend the data scope by reaching non-owner nodes * Remove node job * Remove pyiron_base and h5io dependencies * [patch] Remove version constraints on storage Tbh, I've forgotten why they were there. Now that we exclusively use pickle, I am cautiously optimistic py3.10 will work fine * 🐛 fix test_docs indentation I screwed up the indentation of the return value
* Remove all storage backends except for pickle * Remove pyiron_contrib dependence and references * Remove h5io_browser dependence * Remove no-bidict workaround It was only necessary for h5io since that can't handle bidict's custom reconstructor * Remove channel recursion workaround It was only necessary for h5io since that can't handle recursion. We keep the purging of connections, because those might actually extend the data scope by reaching non-owner nodes * Remove node job * Remove pyiron_base and h5io dependencies * [patch] Remove version constraints on storage Tbh, I've forgotten why they were there. Now that we exclusively use pickle, I am cautiously optimistic py3.10 will work fine * 🐛 fix test_docs indentation I screwed up the indentation of the return value * [minor] Remove registration and NodePackage Just import and use nodes like any other python class. * Extend error message * Remove print
Some stuff slipped through the cracks in earlier PRs
* Refactor storage test * Remove empty line * Rename workflow in tests With the introduction of a pickle backend, this test wound up failing. I haven't been able to work out exactly why, but since pickle is much faster my guess is that something in the test parallelization gets in a race condition with deleting a saved workflow from another test. Simply renaming it here solved the issue, which is an innocuous change. * Introduce `pickle` as a backend for saving * Fix root cause of storage conflict Ok, the reason workflow storage tests were failing after introducing pickle was that the original version of the test had the wrong order of the try/finally scope and the for-loop scope. I got away with it earlier because of interplay between the outer-most member of the loop and the default storage backend. Actually fixing the problem is as simple as ensuring the "finally delete" clause happens after _each_ loop step. This does that and reverts the renaming of the workflow. * Again, correctly order try/finally and for-loops
* Refactor storage test * Remove empty line * Rename workflow in tests With the introduction of a pickle backend, this test wound up failing. I haven't been able to work out exactly why, but since pickle is much faster my guess is that something in the test parallelization gets in a race condition with deleting a saved workflow from another test. Simply renaming it here solved the issue, which is an innocuous change. * Introduce `pickle` as a backend for saving * Fix root cause of storage conflict Ok, the reason workflow storage tests were failing after introducing pickle was that the original version of the test had the wrong order of the try/finally scope and the for-loop scope. I got away with it earlier because of interplay between the outer-most member of the loop and the default storage backend. Actually fixing the problem is as simple as ensuring the "finally delete" clause happens after _each_ loop step. This does that and reverts the renaming of the workflow. * Again, correctly order try/finally and for-loops * Remove keyword argument from pure-decorator You're only supposed to use it as a decorator to start with, so the kwarg was senseless * Add factory import source This is necessary (but not sufficient) to get `as_dataclass_node` decorated classes to pickle. The field name is a bit off compared to `Function` and `Macro`, as now we decorate a class definition instead of a function definition, but it's close. * Bring the dataclass node in line with function and macro * Leverage new pyiron_snippets.factory stuff to find the class * Mangle the stored dataclass qualname so it can be found later * Add tests * Update docs examples to reflect new naming * Update snippets dependency * [dependabot skip] Update env file * Use new pyiron_snippets syntax consistently * Expose `as_dataclass_node` in the API Now that it's pickling as well as anything else * Format black --------- Co-authored-by: pyiron-runner <pyiron@mpie.de>
* Refactor storage test * Remove empty line * Rename workflow in tests With the introduction of a pickle backend, this test wound up failing. I haven't been able to work out exactly why, but since pickle is much faster my guess is that something in the test parallelization gets in a race condition with deleting a saved workflow from another test. Simply renaming it here solved the issue, which is an innocuous change. * Introduce `pickle` as a backend for saving * Fix root cause of storage conflict Ok, the reason workflow storage tests were failing after introducing pickle was that the original version of the test had the wrong order of the try/finally scope and the for-loop scope. I got away with it earlier because of interplay between the outer-most member of the loop and the default storage backend. Actually fixing the problem is as simple as ensuring the "finally delete" clause happens after _each_ loop step. This does that and reverts the renaming of the workflow. * Again, correctly order try/finally and for-loops * Remove keyword argument from pure-decorator You're only supposed to use it as a decorator to start with, so the kwarg was senseless * Add factory import source This is necessary (but not sufficient) to get `as_dataclass_node` decorated classes to pickle. The field name is a bit off compared to `Function` and `Macro`, as now we decorate a class definition instead of a function definition, but it's close. * Bring the dataclass node in line with function and macro * Leverage new pyiron_snippets.factory stuff to find the class * Mangle the stored dataclass qualname so it can be found later * Add tests * Update docs examples to reflect new naming * Update snippets dependency * [dependabot skip] Update env file * Use new pyiron_snippets syntax consistently * Expose `as_dataclass_node` in the API Now that it's pickling as well as anything else * [patch] Fall back on cloudpickle When the pickle backend fails * Format black --------- Co-authored-by: pyiron-runner <pyiron@mpie.de>
* Refactor storage test * Remove empty line * Rename workflow in tests With the introduction of a pickle backend, this test wound up failing. I haven't been able to work out exactly why, but since pickle is much faster my guess is that something in the test parallelization gets in a race condition with deleting a saved workflow from another test. Simply renaming it here solved the issue, which is an innocuous change. * Introduce `pickle` as a backend for saving * Fix root cause of storage conflict Ok, the reason workflow storage tests were failing after introducing pickle was that the original version of the test had the wrong order of the try/finally scope and the for-loop scope. I got away with it earlier because of interplay between the outer-most member of the loop and the default storage backend. Actually fixing the problem is as simple as ensuring the "finally delete" clause happens after _each_ loop step. This does that and reverts the renaming of the workflow. * Again, correctly order try/finally and for-loops * Remove keyword argument from pure-decorator You're only supposed to use it as a decorator to start with, so the kwarg was senseless * Add factory import source This is necessary (but not sufficient) to get `as_dataclass_node` decorated classes to pickle. The field name is a bit off compared to `Function` and `Macro`, as now we decorate a class definition instead of a function definition, but it's close. * Bring the dataclass node in line with function and macro * Leverage new pyiron_snippets.factory stuff to find the class * Mangle the stored dataclass qualname so it can be found later * Add tests * Update docs examples to reflect new naming * Update snippets dependency * [dependabot skip] Update env file * Use new pyiron_snippets syntax consistently * Expose `as_dataclass_node` in the API Now that it's pickling as well as anything else * [patch] Fall back on cloudpickle When the pickle backend fails * [minor] Make pickle the default storage backend * Format black * Fall back on loading any storage contents Regardless of what the specified storage backend was. * Format black --------- Co-authored-by: pyiron-runner <pyiron@mpie.de>
I must have screwed up the import line when doing the rebase merge on GitHub
* [patch] Don't touch storage (#421) * Remove unused import * Never interact directly with the storage object Always go through HasStorage methods. In practice, this was only .delete. * Break the recursive ownership between HasStorage and StorageInterface (#423) * [patch] Merge `HasStorage` back into `Node` (#424) * Refactor: merge HasStorage back into Node * Indicate that StorageInterface is abstract * Refactor: Move storage out of the mixin submodule * 🐛 pass node through methods We were never getting to the cloudpickle deletion branch * Format black --------- Co-authored-by: pyiron-runner <pyiron@mpie.de> * [minor] Checkpointing (#425) * 🐛 compare like-objects * Distinguish saving and checkpointing * Rename flag to reflect checkpoint behaviour * Reflect save/checkpoint difference in StorageInterface It only needs save now -- checkpointing is the responsibility of the node itself * [minor] No `storage` property (#426) * Expose the backend as a node method kwarg * Use the backend kwarg directly in tests * Expose backend in storage deletion too In case someone has their own custom backend * Remove the storage property And route the `storage_backend` kwarg to the post-setup routine * Format black * Allow the backend to be specified for checkpoints * Update storage_backend docs and type hint * Use storage_backend kwarg as intended in tests Including a couple bug-fixes, where I was looping over backends but forgot to pass the loop value to the save method! * 🐛 fix JSON edit typo * Check for stored content to autoload Even if the autoload back end is a custom interface * Give Node.delete_storage consistent use of the backend arg * Pass backend iterator variable to deletion method in tests * Remove unused method * Allow specifically mentioned backends to pass on missing files * Refactor: remove code duplication * Format black --------- Co-authored-by: pyiron-runner <pyiron@mpie.de> * [minor] Autoload (#427) * Refactor: rename storage_backend to autoload * Update autoload default behaviour on the base class And note the difference in Workflow * Add a test to demonstrate the issue And ensure that, for macros at least, the default behaviour stays safe * Format black --------- Co-authored-by: pyiron-runner <pyiron@mpie.de> * [minor] Housekeeping (#428) * Clean up after macro test * Refactor: rename overwrite_save to delete_existing_savefiles * 🐛 Actually deactivate auto-loading for Node * Remove void __init__ implementation It just passes through * Consistent and sensible kwarg ordering, and a doc * Refactor: rename run_after_init to autoload It's shorter and, IMO, better in line with the other kwarg names like "autoload" * Delete storage in the order the nodes are instantiated Otherwise if it fails before hard_input is defined, the finally clause never deletes the self.n1 storage because there's a second failure _inside_ finally * Simplify setup and allow loading and running at init (#429) * [minor] Shift responsibility (#430) * Remove unused property * Shorten variable name * Refactor: rename method * Call base method and remove specialist * Condense has_contents * Remove unused property * Move methods from Node to StorageInterface * Have StorageInterface.load just return the new instance * Break dependence on Node.working_directory * Refactor: rename obj to node in the storage module Since it is the interface for Node instances * 🐛 pass argument * Remove tidy method * Clean up using pathlib * Rely on pathlib This is almost exclusively because `pyiron_snippets.files.DirectoryObject` has no option to avoid the `self.create()` call at instantiation. That means if I want to peek at stuff, I need to create then clean it, which is killer slow. I'm very open to re-introducing `files` dependence again in the future, I just don't have the time to implement the necessary fixes, with test, and go through the release cycle. This is all under-the-hood, so should be safely changeable with a `pyiron_workflow` patch change later anyhow. * Remove the post-setup cleanup No longer necessary now that storage isn't creating directories willy nilly * Simplify and remove unused * Allow semantic objects to express as pathlib.Path * Leverage new semantic path for storage * Remove unneeded exception * Remove unused imports * Use a generator in the storage module for available backends * Test loading failure * Fix docstring * Format black * Remove unused import * Don't overload builtin name --------- Co-authored-by: pyiron-runner <pyiron@mpie.de> * [minor] interface flexibility (#432) * Expose existing kwargs on the node methods * Refactor: slide * Allow storage to specify either a node xor a filename directly * Add tests * Expose backend-specific kwargs And use them for the PickleStorage * Add storage to the API * Pass kwargs through the node interface to storage * Add docstrings * Refactor: rename methods * Shorten kwargs docstrings * Format black * Deduplicate test class name * Remove unnecessary passes Codacy actually catching real things for once --------- Co-authored-by: pyiron-runner <pyiron@mpie.de> --------- Co-authored-by: pyiron-runner <pyiron@mpie.de>
# Conflicts: # .binder/environment.yml # .ci_support/environment.yml # .ci_support/lower_bound.yml # docs/environment.yml # notebooks/deepdive.ipynb # pyiron_workflow/mixin/storage.py # pyiron_workflow/node.py # pyiron_workflow/nodes/composite.py # pyiron_workflow/nodes/for_loop.py # pyiron_workflow/nodes/macro.py # pyiron_workflow/nodes/static_io.py # pyiron_workflow/workflow.py # pyproject.toml # tests/unit/nodes/test_macro.py # tests/unit/test_node.py # tests/unit/test_workflow.py
The one remaining use case was to generate a local subdirectory for `Node.draw`, but this is easy enough to replace with `Semantic.as_path()` I snuck in some docstring edits too
* Update overview * Update readme * 🐛 pass body node executor when provided at init * Update quickstart * 🐛 facilitate dataclass subclassing This is actually a weird issue with python more than us, where a class inheriting from a dataclass passes `dataclasses.is_dataclass`, but doesn't act like one. * 🐛 fix readme example whitespace * 🐛 don't let channel readiness be impacted when type checking is off * Remove unused import * Expose the custom storage error as part of the API * Rewrite the deepdive * Update Node docs * Update node subclass docstrings * Fix readme demo inconsistencies * Clean up the save file produced by the deepdive
* [minor] Replace `HasToDict` mixin with `HasStateDisplay` mixin Functionality is similar -- the point is to get a nice JSON representation for child classes. But the public interface is different (hence minor flag), and in particular we don't use `to_dict` which might interfere with other pyiron activities. There's also some changes to exactly the display looks like, but since it's just a nice/useful thing for humans to look at, this is non-critical and can be improved with patches as needed. * Remove unused import
In optimistic anticipation of tracking the connection of input and output without using an explicit cache (e.g. traitlets to track when channel values change)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I want to release a patch version before the next minor bump, to facilitate easy conversion from existing storage formats to pickle. This then serves as a landing platform for all the work after that which will require a minor bump.
pyiron_workflowdepends onpyiron_atomisticsandpyiron_baseleading to a large number of dependencies #46working_directorydoes not play nicely with macros #316HasToDict[minor] ReplaceHasToDictmixin withHasStateDisplaymixin #435Make fetching and pushing signals private inputs to run, so you can still break the "truth" of a graph state, but you really need to want toThis is complicated and maybe even undesirable. I've come around to the thinking that rather it should be very easy to tell whether the current input to a node is synchronized with the current output of that node, or whether it needs to berun. This is easier and closely related tocache_hit. This afternoon I've been playing around with usingtraitletsto catch desynchronizations of IO. It's not ready now because traitlets uses__getstate__in some ways that interfere with what I'm doing, but I think it's promising. For now, I'll just makeNode.cached_inputsprivate so that any switch in this direction can stay a patch change.