perf!: replace merged data model YAML with JSON for faster loading - #933
perf!: replace merged data model YAML with JSON for faster loading#933oboehmer wants to merge 7 commits into
Conversation
oboehmer
added
tech-debt
|
Hey @oboehmer, thank you for raising this — the performance analysis is solid and the 1.9s → 0.017s per subprocess improvement is compelling, especially at scale (322s saved on a 169-test device). One consideration before we go all-in on JSON: the merged data model file isn't purely a machine-to-machine artifact. Engineers troubleshooting test failures will open that file to understand what values the framework actually used to render their tests. YAML is significantly more readable for that — no braces, no mandatory quotes, clean indentation on deeply nested network config. 2.3MB of JSON is painful to visually inspect. I think the right tradeoff is: JSON by default for speed, with an opt-in to also write a human-readable YAML copy for debugging. Something like This would also cleanly supersede PR #898's sidecar approach — same performance win, simpler implementation, and the human-readable format is available when you need it without forcing anyone to install a separate tool. What do you think? P.S. — This comment was drafted using voice-to-text via Claude Code. If the tone comes across as overly direct or terse, please know that's just how it tends to phrase things. No offense or criticism is intended — this is purely an objective technical review of the PR. Thanks for understanding! 🙂 |
|
Thanks for the comment, @aitestino .. I initially discounted the serviceability aspect as we have removed the file anyway after the run, but a |
The merged data model is now written and read as JSON instead of YAML. json.dump/json.load replaces ruamel's pure-Python YAML parser, which in one observation reduced per-test data model load time from ~1.9s to ~0.017s on a 2.3 MB file. MERGED_DATA_FILENAME constant value updated to .json; all consumers, tests, fixtures, and documentation updated accordingly. result.yaml fixture replaced with result.json. Closes #931
…YAML Optionally write the merged data model as a companion YAML file alongside the JSON, gated by the NAC_TEST_DUMP_YAML_DATA_MODEL env var, for post-run inspection/debugging. The YAML file is not registered for cleanup (persists after the run) and may contain sensitive values, so it is written with 0o600 permissions and a warning is logged advising manual removal. - add dump_yaml parameter to DataMerger.write_merged_data_model() - wire NAC_TEST_DUMP_YAML_DATA_MODEL constant through the CLI - unit test for JSON/YAML content parity - subprocess integration test verifying the YAML persists after exit - assert no YAML by default in existing render test - document env var in README and CHANGELOG
The merged data model file has been JSON since 383c957 (perf!: replace merged data model YAML with JSON). Update two stale docstrings that still described it as YAML.
The merged data model file is JSON (parsed via json.load in base_test). Update remaining test fixtures that created or referenced it as YAML so they match the real format: - rename dummy .yaml files to .json - write JSON content instead of YAML
…st.py The PyATSTestDirs NamedTuple and pyats_test_dirs fixture were duplicated identically in tests/pyats_core/conftest.py and tests/unit/conftest.py. Move both to the top-level tests/conftest.py so they are shared across the whole suite, and repoint the seven importers accordingly.
5bf1409 to
0d5f249
Compare
json.dump now uses default=str so values ruamel's safe loader yields that JSON cannot natively encode (e.g. datetime.date from an unquoted YAML date) are stringified instead of raising TypeError and aborting the run at merge time. device_inventory discovery now reads the merged model with json.load instead of yaml.safe_load, matching base_test and making the JSON format contract explicit (it previously worked only because JSON is a YAML subset, via the slow ruamel path this migration set out to eliminate). Document the two JSON serialization differences versus the previous YAML format in the CHANGELOG breaking-change entry: non-string mapping keys (e.g. integer VLAN IDs used as keys) are coerced to strings, and non-JSON-native values are written in string form. Add unit tests covering both behaviors.
2 participants
Description
Replaces the internal merged data model file format from YAML to JSON. The merged data model is a temporary file passed between the nac-test orchestrator and test subprocesses via the
MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATHenvironment variable. Switching from ruamel's pure-Python YAML parser tojson.dump/json.loadsignificantly reduces per-test setup time. In one observation on a 2.3 MB file, data model load time dropped from ~1.9s to ~0.017s per test subprocess.Closes
Related Issue(s)
Type of Change
Test Framework Affected
Network as Code (NaC) Architecture Affected
Platform Tested
Key Changes
nac_test/core/constants.py:MERGED_DATA_FILENAMEvalue changed from.yamlto.jsonnac_test/data_merger.py:write_merged_data_model()now usesjson.dump()instead ofnac_yaml.write_yaml_file()nac_test/pyats_core/common/base_test.py:load_data_model()now usesjson.load(),safe_loadimport removedTesting Done
pytest/pre-commit run -a)Test Commands Used
pytest tests/unit/ tests/integration/ -n auto --dist loadscope -q \ --ignore=tests/integration/test_integration.py \ --ignore=tests/integration/test_integration_robot_pabot.py # 1008 passedChecklist
pre-commit run -apasses)Screenshots (if applicable)
N/A
Additional Notes
This change is only breaking for consumers that read the merged data model file directly via
MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATHusing a YAML parser. Those consumers need to switch tojson.load(). The standardself.data_modelAPI requires no changes.JSON serialization caveats vs. the previous YAML format: non-string mapping keys (e.g. integer VLAN IDs used as keys) are coerced to strings, and values JSON can't represent natively (e.g. an unquoted YAML date parsed as
datetime.date) are written in string form viajson.dump(..., default=str). This only affects consumers reading the file directly; theself.data_modelAPI for typical string-keyed models is unaffected. Quote such keys/values in your data files if you need them preserved verbatim.PR #898 proposed a JSON sidecar cache approach to solve the same performance problem. This PR takes a simpler path: JSON as the sole format, no sidecar logic, no mtime checks, no atomic tmp writes.
Additional Changes in this PR (beyond the JSON migration)
Building on the JSON migration, this branch also includes:
NAC_TEST_DUMP_YAML_DATA_MODEL: optional env var to also write the merged data model as a YAML file alongside the JSON, for post-run inspection/debugging. Not auto-cleaned up; may contain sensitive values (documented in README + CHANGELOG). Covered by a unit test (JSON/YAML content parity) and a subprocess integration test (YAML persists after run).PyATSTestDirstype andpyats_test_dirsfixture into the top-leveltests/conftest.py.Follow-up
pyats_test_dirsintest_end_to_end_controller_detectionand remove deadmerged_filesetup intest_combined_orchestrator_controller.py(identified during this PR, deferred as out-of-scope).