Skip to content

Reading lockfiles if specified in the runcard #818

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
siranipour wants to merge 2 commits into from
Closed

Reading lockfiles if specified in the runcard #818

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe48936
Reading lockfiles if specified in the runcard
Jun 30, 2020
9105785
Assigning default value of None to theoryID
Feb 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions validphys2/src/validphys/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1068,7 +1068,11 @@ def produce_rules(
if use_cuts is not CutsPolicy.INTERNAL:
return None
if default_filter_rules_recorded_spec_ is not None:
# For when using a lockfile as a runcard
filter_rules = default_filter_rules_recorded_spec_[default_filter_rules]
elif default_filter_rules is not None:
# For when a rules spec is specified
filter_rules = self.load_default_default_filter_rules(default_filter_rules)
else:
filter_rules = default_filter_rules_input()
Comment on lines +1069 to 1073
Copy link
Contributor

@wilsonmr wilsonmr Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I right in thinking that these filter rules in validphys.cuts.filters.yaml are the exact same as in validphys.cuts.lockfiles.31_filters.lock.yaml?

if so I really think this block should be (please double check indents are correct, editing on github is horrible):

Suggested change
elif default_filter_rules is not None:
# For when a rules spec is specified
filter_rules = self.load_default_default_filter_rules(default_filter_rules)
else:
filter_rules = default_filter_rules_input()
else:
if default_filter_rules is None:
# no defaults file specified, use <whatever>
default_filter_rules = self.parse_default_filter_rules("<whatever>")
# save loaded defaults to lockfile.
filter_rules = self.load_default_default_filter_rules(default_filter_rules)

If the defaults are not the same then make a legacy defaults lockfile and read that instead. That way if the defaults ever get changed then the lockfile still contains a record of what they were at the time.Also in the future we can change the default to be 40 or whatever we have come up with by then and old lockfiles still remain valid.

Copy link
Contributor Author

@siranipour siranipour Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean validphys.cuts.filters.yaml at the moment is identical to validphys.cuts.lockfiles.31_filters.lock.yaml, but it need not be in general.

The way I thought it was meant to be was that we have a copy of the 3.1 rules, but the current iteration of the rules is always in validphys.cuts.filters.yaml and we make a new lock file at every release, e.g 4.0 or 4.1 etc...

Copy link
Contributor

@wilsonmr wilsonmr Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it? I did a hasty print and they looked slightly different but I didn't bother working out how (the prints were different lengths).

I don't see the advantage to have some defaults loaded outside of the lockfile system when you have implemented the lockfile system. If I run a report where I use cuts internal but don't specify the set of defaults to use then it falls back on this. But those defaults never get saved to the lockfile for that particular report. So in the future if anybody ever edits validphys.cuts.filters.yaml then the lockfile for my report is invalidated.

Also if you use lockfiles to load the fallback option then you can even change what the fallback option is (now it's 3.1 maybe but in the future could be something else) but all lockfiles will still tell you exactly what set of rules were used at the time.

Copy link
Contributor Author

@siranipour siranipour Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes they're actually different because some datasets were being added as we moved towards 4.0.

Okay I'll commit this suggestion, my heads a bit of a spin with the amount of defaults im seeing.

Copy link
Contributor

@wilsonmr wilsonmr Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If validphys.cuts.filters.yaml has been edited whilst also being the fallback default then do you see how that demonstrates why I think it should be a lockfile?

Copy link
Contributor Author

@siranipour siranipour Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah makes sense. Where should we specify the fallback default then? Some sort of global variable?

Copy link
Contributor Author

@siranipour siranipour Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, when would validphys.cuts.filters.yaml ever be used then?

Copy link
Contributor

@wilsonmr wilsonmr Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh this quickly gets messy and confusing and my suggestion above still isn't the right way to do things. I think the best demonstration of how to do the fallback is with the data_grouping mechanism. That's all handled in its own production rule but you can apply the same ordering and logic in this function.

  1. So first you check if filter_rules is None, if not you can use that (which is already done)
  2. Else Check if default_filter_rules is None, if it is then the user didn't specify it and we must set it such that the fallback defaults is loaded, I'm never good with names but probably that should be called current or something better - see the note on this at the end of the next point though
  3. Check if default_filter_rules_recorded_spec_ is None, if not then we are running from a lockfile and we return the defaults recorded at the time. The point is that we need to have a value for default_filter_rules because we use: filter_rules = default_filter_rules_recorded_spec_[default_filter_rules] if the lockfile was produced from a fallback then we need to set default_filter_rules to be the fallback key. Also this highlights that the fallback key must never change or else old lockfiles will be broken, in that sense I don't think the value of the fallback default_filter_rules should be global, and perhaps there should be a comment next to it saying don't change this.
  4. Else we load the defaults as per filter_rules = self.load_default_default_filter_rules(default_filter_rules) which ensures they get saved to the lockfile.

Just to reiterate, the defaults associated with the fallback value of default_filter_rules can be changed (i.e the contents of the yaml file) however the actual value must never change i.e if default_filter_rules falls back to current now then it basically always has to. In the same way as the data grouping must now always fallback to standard_report

A minor comment is that the files from which the defaults are being loaded from i.e validphys.cuts.lockfiles.31_filters.lock.yaml are not lockfiles, they are just the defaults. The lockfiles are where those defaults get saved.

Copy link
Contributor

@wilsonmr wilsonmr Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validphys.cuts.filters.yaml would be moved to validphys.cuts.lockfiles.<whatever the fallback default_filter_rules value is>_filters.lock.yaml and would get loaded whenever default_filter_rules is not specified.

Copy link
Contributor Author

@siranipour siranipour Feb 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @Zaharid


Expand Down Expand Up @@ -1148,12 +1152,17 @@ def produce_defaults(
raise ConfigError("w2min defined multiple times with different values")

if default_filter_settings_recorded_spec_ is not None:
# If running on a lockfile
filter_defaults = default_filter_settings_recorded_spec_[
default_filter_settings
]
# If we find recorded specs return immediately and don't read q2min and w2min
# from runcard
return filter_defaults
elif default_filter_settings is not None:
# If the user requests to read from a pre existing settings lockfile
filter_defaults = self.load_default_default_filter_settings(default_filter_settings)
defaults_loaded = True
elif not filter_defaults:
filter_defaults = default_filter_settings_input()
defaults_loaded = True
Comment on lines +1166 to 1172
Copy link
Contributor

@wilsonmr wilsonmr Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise. Let's keep a record of all the defaults.

Expand Down