Process Commands #198
Merged
Process Commands #198
Conversation
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
eagmon
reviewed
May 27, 2022
We don't expect users to be changing public Process attributes like Process.parameters. This commit introduces public getters to communicate that these are read-only attributes.
eagmon
reviewed
May 30, 2022
eagmon
reviewed
May 30, 2022
Instead of holding ParallelProcess objects in a separate Engine.parallel dictionary, put them in place of their associated Process objects in Engine.processes, Engine.process_paths, and Engine.state. This also involves removing InvokeProcess and the `invoke` keyword from the Engine constructor. While these are public, the `invoke` keyword was not documented, and use of InvokeProcess is not discussed in our documentation. Therefore, neither are part of our supported API, so removing them does not constitute a breaking change.
U8NWXD
added a commit
to CovertLab/vEcoli
that referenced
this pull request
Jun 13, 2022
Requires PR vivarium-collective/vivarium-core#198, which introduces process commands. The process commands interface lets us retrieve internal state from parallelized EngineProcess instances.
eagmon
approved these changes
Jun 14, 2022
There was a problem hiding this comment.
This is looking really good! I have no improvements. Engine is actually simpler now in many ways. Process is a bit more complicated, but I think it all makes sense and is required.
@prismofeverything -- Can you give this a look, since it is a major design decision?
prismofeverything
approved these changes
Jun 14, 2022
With the move to Process Commands, we stopped parallelizing processes in `Engine._invoke_process` and instead performed parallelization in the constructor. This had the unintended effect of not parallelizing processes with the `parallel` attribute if they were produced by division. This commit ensures that these processes are parallelized as well.
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.
Introduce process commands to support more interactions with parallel processes. Now all
Processmethods of a parallelized process can be queried from the parent OS process. Users can also add support for custom methods of their processes.This change also simplifies the way
Enginehandles parallel processes warns users when serializers are not being found efficiently.TODO:
Engineto putParallelProcessinstances in the store hierarchyinvoke_processfunction that doesn't do anythingBy creating this pull request, I agree to the Contributor License
Agreement, which is available in
CLA.mdat the top level of thisrepository.