-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: support passing extract_data for container and local simulations #525
Conversation
powersimdata/data_access/launcher.py
Outdated
@@ -195,18 +218,28 @@ def _launch(self, threads=None, solver=None, extract_data=True): | |||
:return: (*dict*) -- contains "output", "errors", "scenario_id", and "status" | |||
keys which map to stdout, stderr, and the respective scenario attributes | |||
""" | |||
sys.path.append(server_setup.ENGINE_DIR) | |||
from pyreisejl.utility import app |
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 assume here that the engine is REISE.jl, which is fine for now. In the future, we might have other engines and we will have to access their own utility functions
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.
Should we move the import statement in the constructor and and have a app
attribute so we can do self.app.launch_simulation(...)
, self.app.extract_scenario(...)
and self.app.check_progress(...)
?
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 use importlib.import_module
to assign it? Not sure if there is another way. I tried just moving the import statement to the constructor as-is, but that resulted in an import error.
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 was trying to avoid to have the same import statement in each function. If it is too complicated or what you propose generate more lines, let's move forward with the current implementation.
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.
Done. Usually prefer the standard imports since they can be detected by refactoring tools, etc, but this use case is not too bad, and already a bit "non standard".
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.
Thanks
Purpose
Changes corresponding to Breakthrough-Energy/REISE.jl#146. This exposes the extract-data parameter to a user who is interacting via the scenario object.
What the code is doing
The extract_data parameter is already part of the signature, but needed to be passed to REISE.jl. Also add implementations for
extract_simulation_output
so it can be called when using docker or local installation.Testing
Launched a simulation using docker and natively, with and without extract_data set.
Time estimate
10 min