Fixes #1072 -Fix PyYAML corrupting unquoted HH:MM:SS values - #1087
Closed
shirasassoon wants to merge 15 commits into
Closed
shirasassoon wants to merge 15 commits into
shirasassoon wants to merge 15 commits into
Conversation
…ployment Sparkcompute.yml was round-tripped through yaml.safe_load / yaml.dump whenever instance_pool_id was present. Under PyYAML's YAML 1.1 rules, unquoted live_pool schedule HH:MM:SS times are parsed as sexagesimal integers (18:00:00 -> 64800), which the Fabric API rejects as an invalid TimeSpan. Parse the YAML only for pool lookup/matching and write the resolved GUID back with a targeted single-line replacement, leaving all other lines byte-for-byte unchanged. Also return the file unchanged when no replacement applies. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes Environment deployments where Setting/Sparkcompute.yml contains unquoted live_pool schedule times that were being corrupted by a PyYAML safe_load/dump round-trip (YAML 1.1 sexagesimal parsing), causing Fabric API validation failures.
Changes:
- Avoid YAML re-serialization by resolving the pool mapping via parsed YAML but applying the
instance_pool_idupdate via a targeted single-line text replacement. - Refactor the pool logic by splitting resolution and replacement into
_resolve_instance_pool_idand_replace_instance_pool_id_line. - Add regression tests to ensure unquoted
HH:MM:SStimes are preserved and no-op cases return the original file unchanged.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/fabric_cicd/_items/_environment.py | Stops YAML round-tripping and introduces targeted instance_pool_id line replacement to prevent schedule time corruption. |
| tests/test_environment_publish.py | Adds regression/unit tests covering preserved live_pool times, unchanged no-op behavior, and the line replacer. |
added 3 commits
August 2, 2026 22:23
…om/microsoft/fabric-cicd into fix-1072-sparkcompute-time-parsing
Alon Yeshurun (ayeshurun)
approved these changes
Aug 3, 2026
The key_value_replace parameter applies replacements to YAML files via a yaml.safe_load / yaml.dump round-trip, which hit the same PyYAML YAML 1.1 sexagesimal bug: unquoted HH:MM:SS values (e.g. live_pool schedule times) were corrupted to base-60 integers, and yaml.dump's default sort_keys also reordered the document. Add a sexagesimal-safe SafeLoader/SafeDumper pair that strips the base-60 branch from the int/float implicit resolvers, so time strings are preserved while normal ints, floats, octal, hex, and binary literals still resolve. Dump with sort_keys=False to keep the original key order. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
shirasassoon
changed the title
The parameter.yml file itself is loaded with plain YAML 1.1, so an unquoted HH:MM:SS value used as a replace_value would be parsed as a base-60 integer (18:00:00 -> 64800) and injected into the deployed item. Rather than silently reinterpret the user's parameter file, detect these values during load and emit an actionable warning telling the user to quote them. Detection composes the node tree and flags scalars the resolver treats as int/float whose raw text is colon-separated; quoted values resolve to strings and are not flagged. Applied to both the base parameter file and extended template files. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Consolidate all YAML loading and dumping behind a single _common/_yaml_safe module so YAML 1.1 base-60 (sexagesimal) time corruption cannot silently reappear (#1072). load_yaml/dump_yaml strip only the int/float sexagesimal branch, preserving HH:MM:SS values as strings end to end. - Add SafeYamlLoader/SafeYamlDumper plus load_yaml/dump_yaml helpers. - Route _parameter/_utils, _parameter/_parameter (_DuplicateKeyLoader now inherits SafeYamlLoader), _check_utils, _config_validator, and the Environment publisher through the shared helpers. - Remove the now-redundant parameter-file sexagesimal warning and its constant, since consistent time-safe loading fixes the value at parse time instead of warning after the fact. - Add tests for the shared helpers and a guardrail test that fails if a raw yaml round-trip call is reintroduced in src/. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The hand-written int/float resolver regexes diverged from PyYAML's defaults: the float exponent branch used an optional sign, so unsigned exponents like -3.2e4 were parsed as floats where stock PyYAML keeps them strings. Derive the resolvers from PyYAML's own patterns instead, removing only the base-60 alternation branch, so behavior is identical to yaml.safe_load for every input except the intended HH:MM:SS case. Add differential tests asserting parity with yaml.safe_load across ints, floats, exponents, hex, octal, binary, timestamps, and collections, plus dump parity for normal data. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
shirasassoon
force-pushed
the
fix-1072-sparkcompute-time-parsing
branch
from
August 6, 2026 08:40
8898365 to
08708bd
Compare
- Guard _strip_sexagesimal_branch against an unexpected PyYAML wrapper
format: raise RuntimeError instead of silently slicing a broken/wrong
regex, so a future PyYAML change fails loudly at import.
- Reuse the source pattern's own re flags instead of hardcoding
re.VERBOSE, for forward compatibility.
- Derive each class's resolver table from its own inherited resolvers
(SafeLoader for the loader, SafeDumper for the dumper) rather than
always from SafeLoader, removing an implicit coupling.
- Document why the naive split('|') is safe for PyYAML's specific
alternation patterns.
- Add tests for the format guard and the dumper resolver source; also
restore the guardrail test that had been accidentally merged into the
dump-parity test.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
shirasassoon
changed the title
Contributor
Author
|
abandoning this approach for now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Deploying an Environment item whose
Setting/Sparkcompute.ymlcontains alive_poolschedule fails withSparkSettingsInvalidClpSchedule(for exampleInvalid custom live pool schedule EndTime: 64800.00:00:00). The root cause is PyYAML's YAML 1.1 behavior: unquotedHH:MM:SSvalues are parsed as base-60 (sexagesimal) integers, soend_time: 18:00:00becomes64800and is re-emitted as a bare integer that the Fabric API interprets as a 64800-day .NET TimeSpan.This corruption occurs on any code path that round-trips YAML through
yaml.safe_load/yaml.dump. An audit of every YAML call site insrc/found two such deployment paths. Rather than patch each site independently, this PR centralizes all YAML handling so the whole bug class is fixed in one place and cannot silently reappear.Centralized handling (
_common/_yaml_safe.py)A new shared module is now the single place YAML is loaded and dumped across the package.
load_yaml/dump_yamluse a sexagesimal-safeSafeYamlLoader/SafeYamlDumperpair that strips only the base-60 branch from the int/float implicit resolvers. Time strings are preserved end to end (loaded as strings, emitted unquoted to match the Fabric portal format) while normal ints, floats, octal, hex, and binary literals still resolve exactly as before.dump_yamldefaults tosort_keys=Falseso key order is preserved.load_yamlaccepts a string, bytes, or an open stream.Every YAML call site is routed through these helpers:
_items/_environment.py) -_process_environment_fileno longer re-serializesSparkcompute.ymlat all. Since this path only mutates one known key, the YAML is parsed (viaload_yaml) solely to look up thespark_poolmapping, and the resolved GUID is written back with a targeted single-line replacement ofinstance_pool_id. Every other line (comments, formatting, schedule times) is preserved byte-for-byte, and the file is returned unchanged when no mapping matches.key_value_replaceparameterization (_parameter/_utils.py) - applies replacements to an arbitrary user-supplied JSONPath, so it genuinely needs to parse and re-dump. It now usesload_yaml/dump_yaml, preserving times and key order. JSON targets were never affected._parameter/_parameter.py) -_DuplicateKeyLoadernow inherits fromSafeYamlLoader, so duplicate-key detection and time-safe loading are combined. This means an unquotedHH:MM:SSused as areplace_valueis now preserved correctly (18:00:00stays a string) instead of being corrupted to64800._common/_check_utils.pyand_common/_config_validator.py- YAML validation reads are routed throughload_yamlfor consistency.Because consistent time-safe loading now fixes the parameter-file value at parse time, the interim "unquoted time" warning added earlier in this PR's history is removed - it would have been redundant and its message (telling users to quote the value) would have been misleading now that the value is handled correctly automatically.
Guardrail
A test greps
src/and fails if a rawyaml.safe_load/yaml.dump(or a bareyaml.loadwithout an explicit safeLoader=) is reintroduced outside the shared module, so this class of corruption cannot creep back in.Tests
tests/test_yaml_safe.py:load_yamlpreserves unquoted times, still parses normal scalars, accepts a stream;dump_yamlemits times unquoted and preserves key order; plus the guardrail test.tests/test_environment_publish.py: unquotedlive_pooltimes survive whileinstance_pool_idis replaced; files unchanged when no replacement applies.tests/test_parameter_utils.py:key_value_replaceon YAML preserves schedule times and key order and still resolves normal int/float/bool types.tests/test_parameter.py: the parameter loader preserves an unquoted time value as a string.Full suite passes (1083 passed, 11 skipped); ruff format and lint are clean. No new dependency added.