Apps macro wps #261
Merged
Apps macro wps #261
Conversation
This file contains 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
| example<metadata_changes>`. | ||
|
|
||
| LFRic Apps tickets will require an LFRic Core source to use. You can do this by checking out an appropriate working copy, and exporting the environment variable `ROSE_META_PATH=/path/to/core`. | ||
|
|
There was a problem hiding this comment.
I'm guessing theres no chance of pointing ROSE_META_PATH at an fcm link?
There was a problem hiding this comment.
No, don't think so. Fortunately, a new app is probably quite a rare happening.
Comment on lines
+6
to
+9
| .. important:: | ||
|
|
||
| When developing Upgrade Macros, they must be tested using a test branch (see :ref:`testing`). | ||
|
|
source/WorkingPractices/macros.rst
Outdated
|
|
||
| Namelist files in application example directories are not currently updated by the Apply Macros script. This feature is intended to be introduced, but for now, developers still need to manually update those files. | ||
|
|
||
| The organisation of LFRic metadata is different from other repositories (UM + Jules) as the metadata is stored with the Science or Application section it is associated with and is then imported by other apps that require it. This helps modularise the LFRic code but complicates macro chains as different apps may require different chains depending on the metadata they import. A simple implementation of macros in this case might require duplication of a particular macro if its metadata is imported multiple times. This is unsatisfactory as it makes mistakes more likely and requires more effort. |
There was a problem hiding this comment.
I feel like explaining the alternative solution adds too much bulk to this section. I suggest stopping at "complicates macro chains."
source/WorkingPractices/macros.rst
Outdated
|
|
||
| The organisation of LFRic metadata is different from other repositories (UM + Jules) as the metadata is stored with the Science or Application section it is associated with and is then imported by other apps that require it. This helps modularise the LFRic code but complicates macro chains as different apps may require different chains depending on the metadata they import. A simple implementation of macros in this case might require duplication of a particular macro if its metadata is imported multiple times. This is unsatisfactory as it makes mistakes more likely and requires more effort. | ||
|
|
||
| To solve this, macros in LFRic Apps are applied using a wrapper script which will read the added macros and combine them in versions.py files where the metadata is imported. Therefore when adding macros, the macro should be added in the versions.py file in the same metadata directory as the metadata it is applying. It will then be shared as appropriate by the ``apply_macros.py`` script. For example, if a change to metadata is made in ``science/gungho/rose-meta/lfric-gungho``, the macro should be added to the ``versions.py`` file in that directory. This will then be copied to other ``versions.py`` files that import gungho metadata, eg. lfric_atm, transport etc. |
There was a problem hiding this comment.
Suggested change
| To solve this, macros in LFRic Apps are applied using a wrapper script which will read the added macros and combine them in versions.py files where the metadata is imported. Therefore when adding macros, the macro should be added in the versions.py file in the same metadata directory as the metadata it is applying. It will then be shared as appropriate by the ``apply_macros.py`` script. For example, if a change to metadata is made in ``science/gungho/rose-meta/lfric-gungho``, the macro should be added to the ``versions.py`` file in that directory. This will then be copied to other ``versions.py`` files that import gungho metadata, eg. lfric_atm, transport etc. | |
| To solve this, macros in LFRic Apps are applied using a wrapper script which will read the added macros and combine them into the versions.py files for the apps where that metadata is imported. Therefore when adding macros, the macro should be added in the versions.py file in the same directory as the metadata change being made. It will then be shared as appropriate by the ``apply_macros.py`` script. For example, if a change to metadata is made in ``science/gungho/rose-meta/lfric-gungho``, the macro should be added to the ``versions.py`` file in that directory. This will then be copied to other ``versions.py`` files that import gungho metadata, eg. lfric_atm, transport etc. |
A couple of suggested tweaks. I also wonder if the "for example" should be a new (indented?) paragraph to lower the size of the block of text.
There was a problem hiding this comment.
Done both all of those suggestions
source/WorkingPractices/macros.rst
Outdated
|
|
||
| .. important:: | ||
|
|
||
| Some complex macro commands may be dependent on the order in which they are applied. If macros are copied by the wrapper script, the order they are applied will always be determined by the reverse metadata import order. For example, lfric_atm imports gungho metadata, which itself imports components/driver. If all 3 sections have an associated macro, then the macro commands would be applied in the order: components/driver, gungho, lfric_atm. |
There was a problem hiding this comment.
Suggested change
| Some complex macro commands may be dependent on the order in which they are applied. If macros are copied by the wrapper script, the order they are applied will always be determined by the reverse metadata import order. For example, lfric_atm imports gungho metadata, which itself imports components/driver. If all 3 sections have an associated macro, then the macro commands would be applied in the order: components/driver, gungho, lfric_atm. | |
| Some complex macro commands may be dependent on the order in which they are applied. As macros are copied by the wrapper script, the order they are applied will always be determined by the reverse metadata import order. For example, lfric_atm imports gungho metadata, which itself imports components/driver. If all 3 sections have an associated macro, then the macro commands would be applied in the order: components/driver, gungho, lfric_atm. |
source/Reviewers/howtocommit.rst
Outdated
|
|
||
| where `vnX.Y_tZZZZ` is the `AFTER_TAG` of the latest upgrade macro and the others are paths to the relevant sources. Apps defaults to the current location. Core and Jules default to reading the `dependencies.sh` file in the Apps source. A copy of `apply_macros.py` is available at `$UMDIR/SimSys_Scripts/lfric_macros`. | ||
|
|
||
| All tickets with Core macros are expected to be linked with Apps. Certain tickets with just Core metadata changes may not have required an Apps branch (although an Apps ticket should be provided). This is fine - just checkout the trunk but there is nothing to merge. The apply_macros script will sort the sharing of the upgrade macro added. |
There was a problem hiding this comment.
Suggested change
| All tickets with Core macros are expected to be linked with Apps. Certain tickets with just Core metadata changes may not have required an Apps branch (although an Apps ticket should be provided). This is fine - just checkout the trunk but there is nothing to merge. The apply_macros script will sort the sharing of the upgrade macro added. | |
| All LFRic Core tickets with macros are expected to be linked with LFRic Apps, though they may not have required an LFRic Apps development branch (although an Apps ticket should be provided). This is fine - if there is no LFRic Apps branch just checkout the LFRic Apps trunk. Then run the apply_macros script as described above and this will share the upgrade macro across both LFRic Apps and LFRic Core as needed. |
maybe make this a "note" box?
Trying to tweak to flow better, but not sure I'm 100% there yet!
There was a problem hiding this comment.
Yeah, I was worried about how a note box would appear in the tab section, within a drop-down menu. But it looks fine. And taken your suggested wording, cheers
source/WorkingPractices/macros.rst
Outdated
|
|
||
|
|
||
|
|
||
| 1. Checkout an LFRic Apps working copy and update the core source in ``dependencies.sh`` if required. |
There was a problem hiding this comment.
"if required" = "if you have LFRic Core changes"?
source/WorkingPractices/macros.rst
Outdated
|
|
||
|
|
||
| 1. Checkout an LFRic Apps working copy and update the core source in ``dependencies.sh`` if required. | ||
| 2. Add your upgrade macros. These **must** be added to the versions.py file associated with the metadata they are changing. |
There was a problem hiding this comment.
Suggested change
| 2. Add your upgrade macros. These **must** be added to the versions.py file associated with the metadata they are changing. | |
| 2. Add your upgrade macros. These **must** be added to the versions.py file in the same directory as the metadata being changed. |
source/WorkingPractices/macros.rst
Outdated
|
|
||
| 1. Checkout an LFRic Apps working copy and update the core source in ``dependencies.sh`` if required. | ||
| 2. Add your upgrade macros. These **must** be added to the versions.py file associated with the metadata they are changing. | ||
| 3. Run the Upgrade Macro script - this **must** be done in a test branch (see :ref:`testing`). This is located in the `SimSys_Scripts github repo <https://github.com/MetOffice/SimSys_Scripts>`_ (at meto an up to date clone is available in $UMDIR/SimSys_Scripts). The syntax for running is: |
There was a problem hiding this comment.
Suggested change
| 3. Run the Upgrade Macro script - this **must** be done in a test branch (see :ref:`testing`). This is located in the `SimSys_Scripts github repo <https://github.com/MetOffice/SimSys_Scripts>`_ (at meto an up to date clone is available in $UMDIR/SimSys_Scripts). The syntax for running is: | |
| 3. Run the Upgrade Macro script - this **must** be done in a test branch (see :ref:`testing`). This is located in the `SimSys_Scripts github repo <https://github.com/MetOffice/SimSys_Scripts>`_ (at meto an up to date clone is available in $UMDIR/SimSys_Scripts). The syntax for running is: |
Suggested change
| 3. Run the Upgrade Macro script - this **must** be done in a test branch (see :ref:`testing`). This is located in the `SimSys_Scripts github repo <https://github.com/MetOffice/SimSys_Scripts>`_ (at meto an up to date clone is available in $UMDIR/SimSys_Scripts). The syntax for running is: | |
| 3. Run the Upgrade Macro script in a test branch (see :ref:`testing`). This is located in the `SimSys_Scripts github repo <https://github.com/MetOffice/SimSys_Scripts>`_ (at meto an up to date clone is available in $UMDIR/SimSys_Scripts). The syntax for running is: |
shifting the emphasis of the test branch to the important box below to help the flow of this point (I first read it as the test branch is located in the simsys scripts github repo!)
There was a problem hiding this comment.
Yep, reads better, cheers
source/WorkingPractices/macros.rst
Outdated
|
|
||
| .. important:: | ||
|
|
||
| **Do not commit changes to a Dev Branch having run the apply_macros script** |
There was a problem hiding this comment.
Suggested change
| **Do not commit changes to a Dev Branch having run the apply_macros script** | |
| **Test branches must be used for running the Apply Macros script. Do not commit the changes made by apply_macros.py to a Dev Branch** |
james-bruten-mo
commented
Nov 28, 2024
source/Reviewers/howtocommit.rst
Outdated
|
|
||
| where `vnX.Y_tZZZZ` is the `AFTER_TAG` of the latest upgrade macro and the others are paths to the relevant sources. Apps defaults to the current location. Core and Jules default to reading the `dependencies.sh` file in the Apps source. A copy of `apply_macros.py` is available at `$UMDIR/SimSys_Scripts/lfric_macros`. | ||
|
|
||
| All tickets with Core macros are expected to be linked with Apps. Certain tickets with just Core metadata changes may not have required an Apps branch (although an Apps ticket should be provided). This is fine - just checkout the trunk but there is nothing to merge. The apply_macros script will sort the sharing of the upgrade macro added. |
There was a problem hiding this comment.
Yeah, I was worried about how a note box would appear in the tab section, within a drop-down menu. But it looks fine. And taken your suggested wording, cheers
| example<metadata_changes>`. | ||
|
|
||
| LFRic Apps tickets will require an LFRic Core source to use. You can do this by checking out an appropriate working copy, and exporting the environment variable `ROSE_META_PATH=/path/to/core`. | ||
|
|
There was a problem hiding this comment.
No, don't think so. Fortunately, a new app is probably quite a rare happening.
source/WorkingPractices/macros.rst
Outdated
|
|
||
| Namelist files in application example directories are not currently updated by the Apply Macros script. This feature is intended to be introduced, but for now, developers still need to manually update those files. | ||
|
|
||
| The organisation of LFRic metadata is different from other repositories (UM + Jules) as the metadata is stored with the Science or Application section it is associated with and is then imported by other apps that require it. This helps modularise the LFRic code but complicates macro chains as different apps may require different chains depending on the metadata they import. A simple implementation of macros in this case might require duplication of a particular macro if its metadata is imported multiple times. This is unsatisfactory as it makes mistakes more likely and requires more effort. |
source/WorkingPractices/macros.rst
Outdated
|
|
||
| The organisation of LFRic metadata is different from other repositories (UM + Jules) as the metadata is stored with the Science or Application section it is associated with and is then imported by other apps that require it. This helps modularise the LFRic code but complicates macro chains as different apps may require different chains depending on the metadata they import. A simple implementation of macros in this case might require duplication of a particular macro if its metadata is imported multiple times. This is unsatisfactory as it makes mistakes more likely and requires more effort. | ||
|
|
||
| To solve this, macros in LFRic Apps are applied using a wrapper script which will read the added macros and combine them in versions.py files where the metadata is imported. Therefore when adding macros, the macro should be added in the versions.py file in the same metadata directory as the metadata it is applying. It will then be shared as appropriate by the ``apply_macros.py`` script. For example, if a change to metadata is made in ``science/gungho/rose-meta/lfric-gungho``, the macro should be added to the ``versions.py`` file in that directory. This will then be copied to other ``versions.py`` files that import gungho metadata, eg. lfric_atm, transport etc. |
There was a problem hiding this comment.
Done both all of those suggestions
source/WorkingPractices/macros.rst
Outdated
|
|
||
| .. important:: | ||
|
|
||
| Some complex macro commands may be dependent on the order in which they are applied. If macros are copied by the wrapper script, the order they are applied will always be determined by the reverse metadata import order. For example, lfric_atm imports gungho metadata, which itself imports components/driver. If all 3 sections have an associated macro, then the macro commands would be applied in the order: components/driver, gungho, lfric_atm. |
|
|
||
| .. tip:: | ||
|
|
||
| The wrapper script will read the ``dependencies.sh`` file in your LFRic Apps working copy and will checkout a temporary copy of the LFRic Core source if required. Some Core metadata changes will also modify the Core rose apps. In this case make sure to also commit these changes back to the core branch. |
There was a problem hiding this comment.
Yeah that's right about the temporary copy.
In the script at the moment (line 980 in the 2nd PR if you wanted a peek) it checks if macros were applied to a temporary checkout of the lfric core source (and doesn't tidy it if there were changes). Do you think it's worth explicitly stating all working copies that need committing?
source/WorkingPractices/macros.rst
Outdated
|
|
||
|
|
||
|
|
||
| 1. Checkout an LFRic Apps working copy and update the core source in ``dependencies.sh`` if required. |
source/WorkingPractices/macros.rst
Outdated
|
|
||
|
|
||
| 1. Checkout an LFRic Apps working copy and update the core source in ``dependencies.sh`` if required. | ||
| 2. Add your upgrade macros. These **must** be added to the versions.py file associated with the metadata they are changing. |
source/WorkingPractices/macros.rst
Outdated
|
|
||
| 1. Checkout an LFRic Apps working copy and update the core source in ``dependencies.sh`` if required. | ||
| 2. Add your upgrade macros. These **must** be added to the versions.py file associated with the metadata they are changing. | ||
| 3. Run the Upgrade Macro script - this **must** be done in a test branch (see :ref:`testing`). This is located in the `SimSys_Scripts github repo <https://github.com/MetOffice/SimSys_Scripts>`_ (at meto an up to date clone is available in $UMDIR/SimSys_Scripts). The syntax for running is: |
There was a problem hiding this comment.
Yep, reads better, cheers
source/WorkingPractices/macros.rst
Outdated
|
|
||
| .. important:: | ||
|
|
||
| **Do not commit changes to a Dev Branch having run the apply_macros script** |
jennyhickson
approved these changes
Nov 28, 2024
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.
Instructions for developers and reviewers for writing and testing Upgrade Macros in LFRic