-
Notifications
You must be signed in to change notification settings - Fork 244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Core] Project-based multistage #11355
Merged
Merged
Changes from 52 commits
Commits
Show all changes
56 commits
Select commit
Hold shift + click to select a range
bad13d2
Create GetFinalData
rubenzorrilla b32fc3b
Restart support
rubenzorrilla f7f3577
Register stages
rubenzorrilla 43e2b55
Registry based instantiation and custom
rubenzorrilla a234fcd
Output validated settings
rubenzorrilla f8a818d
Remove useless comment
rubenzorrilla 2c21077
Fix checkpoint list
rubenzorrilla 9cc4215
More descriptive error
rubenzorrilla aaa5072
Discussion changes
rubenzorrilla 9eabcab
Merge branch 'master' into core/multistage-project
rubenzorrilla 8414844
FinalData agreed name
rubenzorrilla ac27c3a
Major advances (not working yet)
rubenzorrilla 5975031
More advances (working)
rubenzorrilla e1a4106
Use registry
rubenzorrilla ca8d87a
Type hints
rubenzorrilla 886748f
Renaming
rubenzorrilla 48ac60b
Merge branch 'master' into core/multistage-project
rubenzorrilla 8914914
Final discussion changes
rubenzorrilla 275821b
Merge branch 'master' into core/multistage-project
rubenzorrilla 3477001
Merge branch 'master' into core/multistage-project
rubenzorrilla af21953
Correct imports
rubenzorrilla 2ef69aa
Merge branch 'master' into core/multistage-project
rubenzorrilla c5adf05
Some of Pooyan's suggestions
rubenzorrilla e45fe13
update helper utils
sunethwarna 8b5dcdd
update stepping analysis
sunethwarna 38e04c0
update independent analysis execution policy
sunethwarna 04a5093
update kratos analysis execution policy
sunethwarna 1f19695
update tests
sunethwarna 0d4d8a1
minor
sunethwarna b915e77
Update applications/OptimizationApplication/python_scripts/utilities/…
sunethwarna 528d9ea
Update kratos/python_scripts/multistage_orchestrators/multistage_orch…
rubenzorrilla 57c7813
Merge pull request #11371 from KratosMultiphysics/optapp/fix_for_ocha…
sunethwarna 64bac3f
Update kratos/python_scripts/project.py
rubenzorrilla 59390eb
Update kratos/python_scripts/project.py
rubenzorrilla 396509c
Update kratos/python_scripts/project.py
rubenzorrilla 85915a0
Update kratos/python_scripts/project.py
rubenzorrilla 52fd8cc
Update kratos/python_scripts/project.py
rubenzorrilla 652e5aa
@philbucher suggestions
rubenzorrilla 6734444
Update kratos/python_scripts/project.py
rubenzorrilla 535577d
Merge branch 'core/multistage-project' of https://github.com/KratosMu…
rubenzorrilla 9c3c6b8
Project type hints completed
rubenzorrilla f3d34e1
Update kratos/python_scripts/multistage_orchestrators/multistage_orch…
rubenzorrilla 495b8bd
Type hints and encapsulating settings
rubenzorrilla 4243919
stage_settings leftowver
rubenzorrilla 62e3e69
Remove multistage keyword from names
rubenzorrilla abf6843
Pooyan's documentation request
rubenzorrilla 41165dd
Test
rubenzorrilla 743d714
Merge branch 'core/multistage-project' of https://github.com/KratosMu…
rubenzorrilla 057a2ab
TODO comment
rubenzorrilla d4c3e35
Adding test to suite
rubenzorrilla 32ae92f
Merge branch 'master' into core/multistage-project
rubenzorrilla 765d021
Missing import in test_KratosCore.py
rubenzorrilla 41405d4
Better documentation
rubenzorrilla 49dabec
Update kratos/python_scripts/orchestrators/orchestrator.py
rubenzorrilla f61a82e
@philbucher's suggestion on the types
rubenzorrilla dd213d7
Import suggestion
rubenzorrilla File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why destroy them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't destroy (i.e. assigning them to
None
) the non pure Python object the class wouldn't be pickable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get the point. A typical use case would be save and continue. No?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. But we are using Python
pickle
for doing so. Pickle is only capable to save / load pure Python types. Hence, we need to convert all Kratos objects to a pure Python type (this is what we do with our stream serializer). Then you "destroy" these objects in order to make the class Pickable. Let me try to write an example.We have a
MyStage
analysis stage class with whatever Kratos object e.g. a Newton-Raphson instance. At this point, the class is not pickable asself.strategy
cannot be dump. In order to do so, we call the stream serializer within theSave
method ofMyClass
to create a string "representing" it. At this point, theMyStage
is still not pickable asself.strategy
is not a Python type but a Kratos one. Then, we need to doself.strategy = None
after the stream serialization. At this point the clas IS pickable. This is what I meant by destroy. In pseudocode will be something likeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://pybind11.readthedocs.io/en/latest/advanced/classes.html#pickling-support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could skip
Load
ing everything back in afterSave
if all members of the analysis stage werepickle
able to begin with (i.e. the bindings define__setstate__
and__getstate__
). That means adding these extra bindings for every C++ type that may end up as a member of anAnalysisStage
.Yes that's a lot of work. However, it's just mundane copy-pasta work. We already have this functionality implemented with serializers, so we could just reuse them for the bindings:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to say that I added more details to the docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I understood
This is exactly my question: Why we should do self.cpp_class_instance=None? Would you please point me to the documentation in this respect?
And this is my direct concern. In the case of this PR the stage is already deleted. But this limits the usage very much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't point you to the docs, because this is just one possible roundabout solution to a problem we created for ourselves, so I don't think the docs would mention anything like this. The followings are just as valid solutions:
self.cpp_class_instance = 0
self.cpp_class_instance = "whatever"
self.cpp_class_instance = 6.0 * 1e23
self.cpp_class_instance = anything_that_is_pickleable
del self.cpp_class_instance
The point is to remove any un
pickle
able member.The proper (albeit more laborious) solution is to make our C++ types
pickle
able.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the thing is more delicate than simple copy-pasting.
Imagine you want to store a Model and then some other python object.
one way of doing this could be to create a serializer object in the pickle pybind call, write it to a string and then write the resulting char array to pickle.
the problem of this approach is that if we now want to have a different save for the second object, we don't have access to the serializer used to save the first object. We could generate a second StreamSerializer insance, however this implies that common things (like the kernel) would be serialized twice (and differently) in the first object and in the second.
What i am trying to say is that we need to first serialize all of the cpp objects USING THE FIRST SERIALIZER, and then write the serialized object using pickle. That is exactly what we are doing.
no doubts we could have a global serializer used in python ... however this complicates things quite a lot.
having said this, this is an implementation detail. If you like, let's open a different thread to discuss about that