read metadata and cards from tar without extracting multi-dim arrays - #563
read metadata and cards from tar without extracting multi-dim arrays#563lol782 wants to merge 2 commits into
Conversation
felixhekhorn
added
enhancement
felixhekhorn
left a comment
felixhekhorn
left a comment
There was a problem hiding this comment.
Remember to run pre-commit
| return content | ||
|
|
||
| @classmethod | ||
| def from_raw(cls, raw: dict) -> "Metadata": |
There was a problem hiding this comment.
Metadata legacy upgrade
yeah, seems that the update_metadata s take paths - but if you look to the function body they don't do anything with it so I'd say we can just remove them from there, because we definitely need patches here
| raise KeyError(f"'{filename}' not found in {tar.name}") | ||
|
|
||
|
|
||
| def load_meta_and_cards_from_tar(path): |
There was a problem hiding this comment.
Function placement
fine for me - any clever suggestion for a shorter function name? 🙃 being explicit is also fine
|
|
||
| def test_load_meta_and_cards_from_tar(self, eko_factory: EKOFactory): | ||
| """Load metadata and both YAML cards from a real EKO archive.""" | ||
| eko = eko_factory.get() |
There was a problem hiding this comment.
Test depth
this is only the current data version - can we please add also all the older as in
eko/tests/eko/io/test_legacy.py
Line 22 in ab85bf7
| def from_raw(cls, raw: dict, data_version: int, theory_raw)-> "OperatorCard": | ||
| """Build an operator card from a raw dictionary.""" | ||
| if data_version == 1: | ||
| raw =v1.update_operator(raw) |
There was a problem hiding this comment.
since you are not testing on the full set of versions you did not realize that this is wrong - look:
Line 65 in ab85bf7
there is a reason I put the argument there 🙃
|
|
||
| Only this member's bytes are read; nothing else is extracted. | ||
| """ | ||
| for member in tar.getmembers(): |
There was a problem hiding this comment.
Is this "costless". If it is, then this is fine.
Otherwise, this function could take a list of filenames so that one can extract all of the files we need in one go.
| raise KeyError(f"'{filename}' not found in {tar.name}") | ||
|
|
||
|
|
||
| def load_meta_and_cards_from_tar(path): |
There was a problem hiding this comment.
| def load_meta_and_cards_from_tar(path): | |
| def load_meta_and_cards_from_tar(eko_path): |
if we are being explicit, better to be explicit here (so that it is clear to the user that this is the "eko path"
|
fix the required changes, Apologies for not remembering pre-commit.
and also changed the name from load_meta_and_cards_from_tar → read_eko_cards and for test function name too. |
felixhekhorn
left a comment
felixhekhorn
left a comment
There was a problem hiding this comment.
- Please mark conversation which you believe to have addressed as resolved, so we know more easily which problems still need to be addressed
- A simple way for me (and everybody else 🙃 ) to see you are human and not a bot is by seeing you thinking. For example e2b6366 could have been 4 commits - doing in turn what you describe here. Making small self-contained commits is not easy and I openly admit I'm not doing it myself all the time, but I try - and this way you can see me thinking.
- this will also help with writing more informative commit messages as "fix the required changes" is not very helpful in the long run
| raw = v1.update_metadata(raw) | ||
| elif version.major == 0 and version.minor == 14: | ||
| raw = v2.update_metadata(paths, raw) | ||
| raw = v2.update_metadata(raw) |
There was a problem hiding this comment.
We want to reuse the new function of course
| raise ValueError(f"Unknown mass scheme '{theory.heavy.masses_scheme}'") | ||
|
|
||
|
|
||
| def _read_yaml_members(tar, filenames): |
There was a problem hiding this comment.
In the last commit you removed types, e.g. -> dict, but please keep them (and consider adding more)
| def legacy_eko_filenames(): | ||
| return ["v1-0.13.tar", "v1-0.14.tar", "v3.tar"] |
There was a problem hiding this comment.
- The "legacy" in the name is not correct since it contains both legacy and current
- rather indicate (some way) that they are actual real objects (and not mocked)
- of course the path should be here (otherwise it is repeated)
|
wait you're still thinking i am a bot 🥲. |
nono, of course not - but I want to help you to avoid that impression PS: and to teach you how to write better commits along the way 🙈 |
3 participants
closes #533
Implements the cheap card-reading we discussed in #533.
Approach (following your instructions )
from_rawtoTheoryCard,OperatorCard, andMetadata(in runcards.py / metadata.py). Each takes raw yaml, applies the v1/v2 upgrades, then callsfrom_dict. This is the upgrade logic lifted out of theEKO.theory_card/EKO.operator_cardproperties.load_meta_and_cards_from_tar(path)in runcards.py: opens the archive and pulls only metadata.yaml / theory.yaml / operator.yaml viatarfile.extractfile— the operators are never extracted. Order is metadata → theory → operator (operator upgrade needs the theory dict).from_raw, so folder-reading and tar-reading share one code path.Points I'd like your view on
v1/v2.update_metadataneeds the folder (paths), which the tar path doesn't have. So I kept that legacy 0.13/0.14 upgrade insideMetadata.loadand madefrom_rawdo the plain build. Consequence: reading a very old EKO straight from tar won't auto-upgrade its metadata. Fine for modern files — is that an acceptable boundary, or do you want it handled differently?load_meta_and_cards_from_tarin runcards.py as you suggested.Existing io tests pass locally and below is the new one.
