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

Scenario Caching #1764

Merged
merged 24 commits into from
Dec 16, 2022
Merged

Scenario Caching #1764

merged 24 commits into from
Dec 16, 2022

Conversation

saulfield
Copy link
Contributor

Closes #1595.

Summary of Changes

This PR makes a number of substantial changes to scenario builds:

  • Scenario build artifacts are now cached and built incrementally, meaning that subsequent builds (without the clean option) will only build the artifacts that depend on the changed DSL objects
  • If the map is a local file in the scenario directory (e.g. a map.net.xml or .xodr file), changes to the map file itself will also trigger a rebuild of artifacts that depend on the map
  • All build artifacts are now in a local build/ directory in each scenario's directory
  • A build.db SQLite database file is now created during builds to keep track of dependencies
  • The allow_offset_map option has been removed. This must now be set by setting shift_to_origin=True in a MapSpec object in the scenario.py file if this option is needed.
  • All scenarios must have a scenario.py, and must call gen_scenario(), rather than the individual gen_ functions. Those functions have been made private, and a deprecation warning will be raised if called from outside of gen_scenario().
  • Empty meshes are now skipped when generating the map GLB file to get rid of noisy trimesh warnings
  • Map GLB file is now only built once (unless the map changes)

Example

Here is the output from a terminal session that demonstrates what a typical iteration workflow looks like now:

# First, clean and build the scenario
$ time scl scenario build --clean scenarios/sumo/minicity/
build-scenario scenarios/sumo/minicity/
INFO:/home/saul/code/SMARTS/smarts/sstudio/genscenario.py:"map_glb" took: 32551.769018ms
INFO:/home/saul/code/SMARTS/smarts/sstudio/genscenario.py:"traffic" took: 8146.712780ms
INFO:/home/saul/code/SMARTS/smarts/sstudio/genscenario.py:"social_agent_missions" took: 5.387068ms
INFO:/home/saul/code/SMARTS/smarts/sstudio/genscenario.py:"bubbles" took: 3.000498ms

real	0m42.086s
user	0m42.094s
sys	0m2.561s

# Then, make a change to the traffic specification in scenario.py, and rebuild (without --clean)
$ time scl scenario build scenarios/sumo/minicity/
build-scenario scenarios/sumo/minicity/
INFO:/home/saul/code/SMARTS/smarts/sstudio/genscenario.py:"traffic" took: 7710.859299ms

real	0m9.068s
user	0m9.289s
sys	0m2.256s

@qianyi-sun
Copy link
Contributor

Hi Saul, just minor, have you tested the this in the replay scenarios (e.g., NGSIM)? I went though your changes/descriptions and the current scenario.py files in the NGSIM folder, looks ok but just wanted to double check.

Also regarding the following point:
"The allow_offset_map option has been removed. This must now be set by setting shift_to_origin=True in a MapSpec object in the scenario.py file if this option is needed."
Should we check all the replay scenarios to see if we should add the shift_to_origin=True if we havent done yet? I can check them once this PR get merged.

Thanks!

CHANGELOG.md Outdated Show resolved Hide resolved
docs/sim/scenario_studio.rst Show resolved Hide resolved
docs/sim/scenario_studio.rst Outdated Show resolved Hide resolved
docs/sim/scenario_studio.rst Outdated Show resolved Hide resolved
scenarios/sumo/figure_eight/scenario.py Show resolved Hide resolved
smarts/core/tests/test_done_criteria.py Show resolved Hide resolved
smarts/core/utils/file.py Show resolved Hide resolved
smarts/core/utils/file.py Show resolved Hide resolved
smarts/core/utils/geometry.py Show resolved Hide resolved
smarts/sstudio/__init__.py Show resolved Hide resolved
@Gamenot
Copy link
Collaborator

Gamenot commented Dec 8, 2022

@Gamenot I need to do a second review specifically on the build changes.

Copy link
Collaborator

@Gamenot Gamenot left a comment

Choose a reason for hiding this comment

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

I just have a few questions.

smarts/sstudio/genscenario.py Show resolved Hide resolved
smarts/sstudio/genscenario.py Show resolved Hide resolved
smarts/sstudio/genscenario.py Show resolved Hide resolved
smarts/sstudio/genscenario.py Show resolved Hide resolved
smarts/sstudio/genscenario.py Outdated Show resolved Hide resolved
smarts/sstudio/genscenario.py Show resolved Hide resolved
@saulfield
Copy link
Contributor Author

Hi @qianyi-sun, getting to your comments just now.

Hi Saul, just minor, have you tested the this in the replay scenarios (e.g., NGSIM)? I went though your changes/descriptions and the current scenario.py files in the NGSIM folder, looks ok but just wanted to double check.

They all build correctly using the new system. I haven't run any examples with them, but there shouldn't be any change.

Also regarding the following point:
"The allow_offset_map option has been removed. This must now be set by setting shift_to_origin=True in a MapSpec object in the scenario.py file if this option is needed."
Should we check all the replay scenarios to see if we should add the shift_to_origin=True if we havent done yet? I can check them once this PR get merged.

Yes that's a good idea. There may be some scenarios that require that change that aren't covered by the tests. If you have some time to check them that would be helpful, thanks!

@Adaickalavan
Copy link
Member

Should we put in a CI test to specifically test the scenario build caching feature?

@saulfield
Copy link
Contributor Author

Should we put in a CI test to specifically test the scenario build caching feature?

Good idea. I added a test in cli/tests/test_studio.py.

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.

4 participants