-
Notifications
You must be signed in to change notification settings - Fork 4
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.
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
manage BIG DATA #255
manage BIG DATA #255
Conversation
log files from the |
note, there's a broken legacy link in the CodeSystemReview template that points to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally good stuff.
These instructions apply across multiple repositories so we need to take care with the LFRic Apps specifics (I don't think we have documentation for updating UM input files either so we'll capture that here too). I've highlighted a few places where a more general statement would be better, and once this is all on then someone from SSD should probably come along and update the UMDIR instructions to round it out too.
This page also has a tendency to get long and overwhelming in detail so I've also tried to pick up on places where the detail could be hidden behind e.g. an expanding box to make it easy for reviewers to find key points.
Thanks
source/Reviewers/howtocommit.rst
Outdated
|
||
**If** your change is known to alter answers, you need to update rose-stem KGO | ||
for all affected tests before you commit to the trunk. | ||
|
||
If supporting data changes or updates are required, then you need to provide updates to BIG_DATA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a more generic statement at this point about "Supporting data is stored in the filesystems of our machines and changes to this will require the reviewer to update those files."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
source/Reviewers/howtocommit.rst
Outdated
4.1 Managing BIG_DATA | ||
^^^^^^^^^^^^^^^^^^^^^^ | ||
|
||
Static input data, such as ancilliaries, are required by many `lfric_apps` tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not just "lfric_apps". Probably just "are required by many tests"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
source/Reviewers/howtocommit.rst
Outdated
path prefix to provide direct access to data required for tests. | ||
|
||
The master copy of this is held on XCS at `/common/lfric/data/`. | ||
A `cron` job is run daily at 07:30 utc on `xcslr0` as the `lfric` user, which runs the script: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The details of the cron should probably be behind an expanding box as not often needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you have a syntax example of an expanding box that I can crib from please @jennyhickson ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're used a few places on this commit page - try the macros section. Cheers
source/Reviewers/howtocommit.rst
Outdated
|
||
Static input data, such as ancilliaries, are required by many `lfric_apps` tests. | ||
|
||
LFRic's lfric_apps tests use a BIG_DATA_DIR environment variable to provide a platform based |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"LFRic Apps" rather than "LFRic's lfric_apps".
Once we add UM/JULES etc info then this, and the details from below about needed access to the lfric user will probably want to be on a tab set per repo as you'll see with other stuff here, but perhaps can be left for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think adding a tab setup, with just the lfric_apps tab, is easy enough here
source/Reviewers/howtocommit.rst
Outdated
|
||
sudo -u lfric -i | ||
|
||
As reviewer, you should work with the developer to: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a) this can probably be generallised a bit - "in charge of the trunk for the repositories involved" to cover all bases.
b) I want to question/think about process here. Holding onto the trunk while waiting for the cron to run feels like it could be wasting peoples time. In the case where we're adding new files I think this is unneeded. Perhaps the following:
- Place new files in the appropriate location (where that location is is per repo so should think about where/how to display that - or perhaps this whole section wants to be on per-repo tabs since the specifics of XCS first are also unique to LFRic)
- Run relevant tests on XCS
- Once the cron job has sychronised the data between s/ef/spice:
- ensure you're in charge of the trunks
- update your working copy if other commits have happened
- run relevant tests on e/f and spice
- proceed to commit
I think the existing files instructions is fine as with overwritting files you will want to make sure others aren't commiting and you've already suggested waiting for the cron to be a bad idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this further - assuming that per repo we've provided details further up on master location and synchronisation versions perhaps this could be generalised to:
-
Place new files in the master location as described above
-
Run tests on that platform
-
Once the files have synchronised to all platforms:
- Ensure you're in charge of the trunks
- Update your working copy if other commits have happened
- Run tests on all other platforms
(I'm also not sure if "proceed to commit" is needed - I'm not sure we have "move to next step" on any other steps on this page).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
understood, i've reworded, to meet some of this, recognising that rewriting with UM in mind will occur
#. Rerun relevant tests on `XCE/F` and `SPICE` | ||
#. Proceed to commit. | ||
|
||
If the requirement is to update existing files, then further care is required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and then (sorry, thinking outloud here!), perhaps instead of a different set of instructions, this could be an "important:: " box with tips like
If you need to update exisiting files then take extra care, such as:
- ensure you are in charge of the trunks before updating any files
- retain a temporary copy of existing files, using a .old suffix
- revert changes immediately if there are any issues and consult with the developer
- manually synchronise the files rather than waiting for a cron to run to avoid introducing a misalignment or race condition on testing
- once happy with all testing remove any temporary files.
i've addressed most of the comments on this; I haven't put in the "perhaps instead of a different set of instructions, this could be an "important:: " box with tips like" open to more chatter on this, of course thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes. I think this is a good starter and we can always tweak further as we add UM based instructions/details too.
This process updated proposes that
BIG_DATA_DIR
is synchronised, withXCS
/common/lfric/data
being treated as the master copy.associated scripting will be managed within
https://github.com/MetOffice/lfric_tools
see
https://github.com/MetOffice/lfric_tools/pull/25
This has not been run or scheduled, pending resolution of this review