Enhanced yaml provenance for autosubmit application#3
Enhanced yaml provenance for autosubmit application#3MuhammadShafeeque wants to merge 33 commits into
Conversation
|
The latest experiment was run with autosubmit branch esm-provenance-to-as in |
|
Thanks for this PR. Before I review it, could you clarify the following questions?
Can you explain why do we need that the provenance survives pickling in Autosubmit? What is Autosumit doing that requires to load a pickle? I have assumed that AS parses the yamls, merges the info inside and then all it does is to use that config. Having pickable provenance is good, but might come with some overhead in the pickling or reading the pickled files, have you profiled how long does it take for a realistic experiment? Please, let me know when it is ready for review and turn off draft mode |
with provenance: real 0m8.822s
user 0m8.437s
sys 0m0.446s
without provenance: real 0m6.175s
user 0m5.814s
sys 0m0.603s |
|
@mandresm the PR is ready for review. |
483688a to
82b918c
Compare
|
I'd like to reduce the 2-3 seconds difference. But again, I need to review the whole paclage and then I should be able to improve its performance. |
mandresm
left a comment
There was a problem hiding this comment.
Thanks for this PR @MuhammadShafeeque. Looks good to me in it's new functionalities and the architectural decisions made. I have a few suggestions for improvement though.
…upport - Added `wrap_computed`, `transfer_provenance`, and `annotate_dict` functions to `_helpers.py`. - Introduced `_serialization.py` for pickle reducers, YAML representers, and a JSON encoder. - Updated `load_yaml` to accept file-like objects. - Added tests for new helper functions and serialization features.
…r pickling support
Co-authored-by: Miguel <63242832+mandresm@users.noreply.github.com>
…r helper functions and load_yaml file-object support
…test for full history transfer
…e; update deepcopy test description
…ontext in CONTEXT.md and rename test file for consistency
…tion and update related imports; register YAML representers at class creation in wrapper and enhance tests for serialization
… of NoneWithProvenance; update test for None handling (debugging after review)
…ce in _dict.py and _helpers.py
e52b7c4 to
35a9d3d
Compare
mandresm
left a comment
There was a problem hiding this comment.
Hi @MuhammadShafeeque, here another round of questions and suggestions
| if value is None: | ||
| return NoneWithProvenance(value, provenance) |
There was a problem hiding this comment.
This was removed from wrapper_with_provenance_factory. If None is never subclassable, then keep that through out the code. Or am I understanding something wrong?
There was a problem hiding this comment.
Both are intentional. YAML-loaded None stays plain None (the factory) because its provenance lives on the key, the loader wraps keys precisely so empty values keep a trace, so is None still works and nothing is lost. wrap_computed/set_provenance inject values whose key isn't provenance-wrapped, so there the value itself must carry provenance via NoneWithProvenance. Added a one-line comment at each site to make the split explicit
There was a problem hiding this comment.
I think this behavior is very confusing. I understand you want is None to be evaluated correctly, but having some Nones be Nones and others be NoneWithProvenance is really inconsistent and dangerous.
I'll have to think about it.
There was a problem hiding this comment.
Please, refer to the comment above for further discussion on the NoneWithProvenance
There was a problem hiding this comment.
Agreed, and fixed. None is now wrapped in exactly one place. The factory (_wrapper.py, elif value is None: return NoneWithProvenance(value, provenance)). Every path (YAML loader, put_provenance, set_provenance, wrap_computed, transfer_provenance) routes through it, so there's no more half-and-half.
Co-authored-by: Miguel <63242832+mandresm@users.noreply.github.com>
Co-authored-by: Miguel <63242832+mandresm@users.noreply.github.com>
Co-authored-by: Miguel <63242832+mandresm@users.noreply.github.com>
Co-authored-by: Miguel <63242832+mandresm@users.noreply.github.com>
…acking in nested structures
…y to clarify provenance handling
…nce to ensure proper memoization
mandresm
left a comment
There was a problem hiding this comment.
This PR at the moment changes turns some Nones to NoneWithProvenance and not others. I'm not okay with this inconsitency and I don't fully understand why it is needed. @MuhammadShafeeque, please see my comments below. IMO this PR is getting very close to a mergable state :)
| val, provenance.get(key, []), config=self._config | ||
| ) | ||
| elif hasattr(val, "provenance"): | ||
| self[key].provenance.extend(provenance.get(key, {})) |
There was a problem hiding this comment.
| self[key].provenance.extend(provenance.get(key, {})) | |
| elif val is None: | |
| # No provenance-wrapped key here (unlike the YAML loader), so carry it on the value. | |
| self[key] = NoneWithProvenance(val, provenance) |
There was a problem hiding this comment.
I went a slightly different way than this literal suggestion, and I think it lands where you wanted: instead of special-casing None here, I restored None-wrapping in the factory (your _dict.py:115 suggestion), so put_provenance's existing else: wrapper_with_provenance_factory(val, provenance.get(key, None)) already produces a NoneWithProvenance. Adding the elif too would re-introduce the per-method special-case that this review is (rightly) getting rid of. Same outcome — the null keeps its provenance — with one code path. Guarded by test_none_value_keeps_provenance. Happy to add the explicit branch back if you'd rather have it spelled out.
| prov = getattr(original, "provenance", None) | ||
| if not prov: | ||
| return result | ||
| return wrapper_with_provenance_factory(result, prov) |
There was a problem hiding this comment.
No support for NoneWithProvenance, but again, I think the factory should be doing that for you
| elif val is None: | ||
| # No provenance-wrapped key here (unlike the YAML loader), so carry it on the value. | ||
| self[key] = NoneWithProvenance(val, provenance) |
There was a problem hiding this comment.
I understand the decision of not having NoneWithProvenance in the factories to conform with PEP 8 and have is None be evaluated correctly. But then why should we keep it here? I would suggest to remove completly NoneWithProvenance or keep it in the factories.
The whole design of the tool is based on keeping provenance in the values, consequently, I'd keep the NoneWithProvenance. I don't have a problem with NoneWithProvenance is None evaluating to False. Maybe this is a problem for AS but then one can simply change in AS my_val_that_it_a_NoneWithProvenance is None or isinstance(my_val_that_it_a_NoneWithProvenance, NoneWithProvenance. One can even make a function out of it (yaml_provenance.is_none_like).
In summary, removing NoneWithProvenance from the wrappers is a decission that goes against the ground design of this package and could have very important implications and lead to missfunction. Please, consider bringing it back.
There was a problem hiding this comment.
Brought it back and centralized it in the factory. That's now the single place NoneWithProvenance is constructed. I also added the exact helper you proposed: yaml_provenance.is_none_like(x) → value is None or isinstance(value, NoneWithProvenance), exported at top level. The special-case that was here in set_provenance is gone (folded into the factory). Value-level provenance for nulls is preserved end-to-end — e.g. empty_value: null now dumps as empty_value: # file,line:2,col:14 instead of "no provenance".
| if value is None: | ||
| return NoneWithProvenance(value, provenance) |
There was a problem hiding this comment.
Please, refer to the comment above for further discussion on the NoneWithProvenance
| return NoneWithProvenance(value, provenance) | ||
| # Return plain None so that `x is None` identity checks work as expected. | ||
| # NoneWithProvenance breaks Python's idiomatic `is None` test (PEP 8). | ||
| # Provenance on a None value is inaccessible anyway (None has no attributes). | ||
| return None |
There was a problem hiding this comment.
As suggested above, I would revert this back.
Please, let me know what are the consequences of reverting this back. There might be alternative solutions to just partially removing NoneWithProvenance from the package.
There was a problem hiding this comment.
I cannot fully confirm the consequences of reverting this yet. It will likely require corresponding changes on the Autosubmit side and validation with a real experiment.
The VM is currently unavailable, so I cannot test it immediately. I will check this later once the VM is available and report back.
Co-authored-by: Miguel <63242832+mandresm@users.noreply.github.com>
…updating wrapper behavior for None values
…d remove redundant imports
Summary
Extends
yaml-provenancewith serialization support,copy.deepcopypreservation, and helper functions needed for real-world integration (e.g. Autosubmit). These additions ensure that provenance-wrapped objects survive pickling, JSON encoding, ruamel.yaml re-dumping, and deep copying without silently losing their provenance metadata.What's new
Helper functions (_helpers.py)
wrap_computed(value, source)— Attach provenance to programmatically injected values (env vars, computed parameters) using a human-readable source string instead of file/line/col.transfer_provenance(original, result)— Re-attach provenance after string operations (str.upper(),str.strip(), etc.) that return plainstr.annotate_dict(d, source_prefix)— Recursively wrap every scalar leaf of a plain dict with per-key provenance, in-place.Serialization (_serialization.py — new module)
register_pickle_reducers()— Patch__reduce__on all WithProvenance classes sopickle.dumps/pickle.loadswork.register_yaml_representers()— Register WithProvenance types on ruamel.yaml'sSafeRepresenterandRoundTripRepresenter.ProvenanceJSONEncoder—json.JSONEncodersubclass that handlesBoolWithProvenanceandNoneWithProvenance.copy.deepcopysupport__deepcopy__to all wrapper types (_wrapper.py),DictWithProvenance(_dict.py), andListWithProvenance(_list.py) so thatcopy.deepcopypreserves provenance instead of silently reducing to plain builtins via__reduce__.YAML dumper improvements (
_yaml_dumper.py)line:andcol:from provenance comments when both are 0 (computed values viawrap_computed).AQUA_SETUP: {}) by falling back to key provenance.YAML loader improvements (
yaml_loader.py).nameattribute) in addition to paths.Files changed
wrap_computed,transfer_provenance,annotate_dict__deepcopy__for dynamic + unsubclassable wrappers__deepcopy__forDictWithProvenance__deepcopy__forListWithProvenanceTesting
All 123 tests pass (
pytest tests/ -q). No new dependencies added.