fix: remove PyYAML runtime dependency from Bazel flow scripts#4113
fix: remove PyYAML runtime dependency from Bazel flow scripts#4113oksaumya wants to merge 2 commits intoThe-OpenROAD-Project:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the runtime PyYAML dependency from the Bazel sandbox execution path by switching flow runtime scripts to consume a checked-in variables.json (generated from variables.yaml), and adds CI tooling to keep the JSON in sync.
Changes:
- Add
flow/scripts/variables.jsonand switchdefaults.py+non_stage_variables.pyto load variables viajson(stdlib) instead of PyYAML. - Add
flow/scripts/yaml_to_json.pyto regeneratevariables.jsonfromvariables.yaml. - Update Bazel packaging and GitHub Actions to include/check the new JSON artifact.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
flow/scripts/yaml_to_json.py |
New developer utility to convert variables.yaml → variables.json. |
flow/scripts/variables.json |
Checked-in JSON representation of flow variables for runtime use without PyYAML. |
flow/scripts/non_stage_variables.py |
Switch runtime parsing from YAML to JSON. |
flow/scripts/defaults.py |
Switch runtime parsing from YAML to JSON. |
flow/BUILD.bazel |
Ensure scripts/*.json is available to Bazel flow targets. |
.github/workflows/github-actions-yaml-test.yml |
Add CI step intended to verify variables.json stays in sync with variables.yaml. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
oksaumya
force-pushed
the
fix-yaml-module-dependency
branch
2 times, most recently
from
c31df6c to
c43b4d8
Compare
|
Developers will still need to install it so this doesn't help them out. Is it hard to simply install it? |
|
@maliberty Good point, I want to keep the developer experience unchanged. The issue is specifically the Bazel sandbox: these scripts are exec'd from Tcl (flow/scripts/util.tcl:162), so they use the sandbox's isolated Python which can't access pip-installed packages. There's no way to pip install into the sandbox. I can simplify the approach though: have the scripts try import yaml first and fall back to import json only when yaml isn't available. That way:
This keeps the developer workflow unchanged while fixing the sandbox issue. Want me to update the PR? |
|
Yes. Claude also suggests
|
non_stage_variables.py and defaults.py are called at runtime via exec from Tcl/Make inside the Bazel sandbox, where the system Python does not have PyYAML installed, causing ModuleNotFoundError. Switch these scripts to use json (stdlib) by reading from a checked-in variables.json instead of variables.yaml. A CI check ensures the JSON stays in sync with the YAML source of truth. Signed-off-by: saumya <saumyakr2006@gmail.com>
oksaumya
force-pushed
the
fix-yaml-module-dependency
branch
from
c43b4d8 to
aacb9b4
Compare
|
@maliberty Updated with all suggestions, narrowed glob to scripts/variables.json, added linguist-generated to .gitattributes, and fixed CI (sparse-checkout, dependency ordering, explicit pyyaml install). |
|
In the future please don't force push after review has started as it breaks the "see changes since last review" feature. |
|
CI error is unrelated |
|
Understood, I’ll avoid |
|
A noticed an issue that I don't think you created but since you are improving this maybe you can fix it. yamlfix check step is passing even though it is flagging differences. That should be a fail. |
- Bump yamlfix from 1.17.0 to 1.19.1 - Add --exit-code to git diff in yamlfix step so CI fails when yamlfix flags formatting differences Signed-off-by: saumya <saumyakr2006@gmail.com>
|
Pushed a new commit with:
|
|
@maliberty could you take a look when you have a moment? Let me know if there’s anything else to do. |
Fixes #4103
Summary
non_stage_variables.pyanddefaults.pyfail withModuleNotFoundError: No module named 'yaml'when running in the Bazel sandbox, where the system Python does not have PyYAMLjson(stdlib) by reading from a checked-invariables.jsonyaml_to_json.pydeveloper tool to regenerate the JSON fromvariables.yamlvariables.jsonstays in sync withvariables.yamlTest plan
python3 flow/scripts/non_stage_variables.py synthworks without PyYAML installedpython3 flow/scripts/defaults.pyworks without PyYAML installedbazel test -c opt test/...in OpenROAD and confirm no moreyamlimport errors