Skip to content
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

Engine configuration utility #1828

Merged
merged 14 commits into from
Feb 10, 2023
Merged

Engine configuration utility #1828

merged 14 commits into from
Feb 10, 2023

Conversation

Gamenot
Copy link
Collaborator

@Gamenot Gamenot commented Feb 2, 2023

This adds an engine configuration utility to SMARTS.

@Gamenot
Copy link
Collaborator Author

Gamenot commented Feb 2, 2023

I am somewhat struggling with figuring out a simple way to propagate the configuration.

  1. Loop through the configuration and use them to set environment variables. (this should pass to subprocesses and workers but it would also mean only a single engine configuration of SMARTS could ever run on the same thread)
  2. Explicitly send over the configuration to workers. This feels like it would be clunky but it would be consistent.
  3. Share an instance id with all workers/subprocesses, this allows reference to a shared resource location, all workers/subprocesses intialize using information from that location.
  4. Create and pass an environment capture object. When it deserializes it restores the global configuration.
  5. Set a series of configuration paths (global user, local user, local directory) that are resolved in order of closeness to the project. Sub-processes will know about it inherently and each lazy loads the configuration independently.
  6. Configure the engine through scl.

@Adaickalavan @saulfield Thoughts?

@Gamenot
Copy link
Collaborator Author

Gamenot commented Feb 6, 2023

Discussion

We had some discussion regarding this issue.

  1. Loop through the configuration and use them to set environment variables. (this should pass to subprocesses and workers but it would also mean only a single engine configuration of SMARTS could ever run on the same thread)

This seems viable, as it may allow us to make the engine configuration propagate across machines if a program propagates across workers (using something like ray) as long as the environment configuration resolves before the workers are generated (before something like ray.init().) It looks like this might be viable to implement.

With that said, any user using a cluster management system could just inline the configuration using environment variables.

  1. Explicitly send over the configuration to workers. This feels like it would be clunky but it would be consistent.

This option is likely too clunky and there is some question about performance. We can enforce sending over the configuration by accessing workers only through utilities we provide. If a user decides not to use a utility they may get confused as to why the engine configuration does not carry over.

  1. Share an instance id with all workers/subprocesses, this allows reference to a shared resource location, all workers/subprocesses intialise using information from that location.

This is probably the most intensive solution, because we would need to provide a central or distributed location for the processes to find the engine settings. We could use an event processing platform like apache kafka or ros, or a cluster management system ray, or a web server. All of these seem like too much work for the immediate need.

  1. Create and pass an environment capture object. When it deserialises it restores the global configuration.

This is very similar to option 2, it still requires us to route the user through an engine utility but it ends up being perhaps even more mysterious to end users.

  1. Set a series of configuration paths (global user, local user, local directory) that are resolved in order of closeness to the project. Sub-processes will know about it inherently and each process lazy loads the configuration independently.

This seems like the most reasonable option for now. The exact implementation will resolve each of the configuration in this priority:

  1. Individual environment variables (e.g. SMARTS_SENSOR_WORKER_COUNT)
  2. Local directory engine configuration (./smarts_engine.ini )
  3. Local user engine configuration (~/.smarts/smarts_engine.ini ) if local directory configuration is not found.
  4. Global engine configuration (/etc/smarts/smarts_engine.ini ) if local configuration is not found.
  5. Package default configuration (${PYTHON_ENV}/lib/${PYTHON_VERSION}/site-packages/smarts/core/smarts_engine.ini).
  1. Configure the engine through scl.

This is viable regardless of other features. It allows us in the case of scl run to provide a --configuration flag which we could pass in a configuration from a custom location and immediately resolve it before anything else is evaluated.

Direction

We decided to go with option 5 and combine it with option 2. At a later date we may want to bring in option 6.

@Gamenot Gamenot force-pushed the tucker/feature-add_configuration branch from a49f476 to 1394667 Compare February 8, 2023 20:02
@Gamenot Gamenot changed the base branch from develop to master February 8, 2023 20:02

import numpy as np

import smarts
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caused an odd bug. It could not resolve the difference between smarts and smarts.core.smarts. I believe should change smarts.core.smarts to smarts.core.engine.

@Gamenot Gamenot changed the title Create the configuration utility Engine configuration utility Feb 9, 2023
docs/index.rst Outdated
Comment on lines 71 to 72
api/modules.rst
sim/configuration.rst
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe keep files under a given toctree in the same folder?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is hard to find where to put this since api/* is generated dynamically but environment variables and configuration files are related to sim but are relevant to API. Maybe not relevant enough. I am going to move it to resources.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking resources is not quite the right spot for it either so I am moving it to next steps.

@Adaickalavan
Copy link
Member

Do check whether the docs build properly without any warnings before merging.

docs/sim/configuration.rst Outdated Show resolved Hide resolved
"max_pybullet_freq",
default=MAX_PYBULLET_FREQ,
cast=int,
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think pulling this out as a separate variable would make the expression more readable.

@Gamenot Gamenot merged commit 41ec4b7 into master Feb 10, 2023
@Gamenot Gamenot deleted the tucker/feature-add_configuration branch February 10, 2023 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants