Skip to content
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

Improve artefact workflow #325

Merged
merged 255 commits into from
Jul 25, 2024

Conversation

hiker
Copy link
Collaborator

@hiker hiker commented Jul 17, 2024

This PR introduces new artefact categories defined as enums. The code (and its typing) supports strings and this enum as keys, meaning any user script can artefacts as they want (and be certain not to interfere with the ones used in Fab).

I then used three new categories for fortan/c/psyclone build files, and updates all steps to keep these categories up-to-date. E.g. when pre-processing a Fortran file, the .F90 would be removed from the fortran-build-files, and the .f90 will be added etc. PSyclone will add the newly created files to the fortran-build-files. This means that e.g. the Fortran analysis step only need to look at two artefacts (C and Fortran), instead of the current one:

DEFAULT_SOURCE_GETTER = CollectionConcat([
    SuffixFilter('all_source', '.f90'),
    'preprocessed_c',
    'preprocessed_fortran',

    # todo: this is lfric stuff so might be better placed elsewhere
    SuffixFilter('psyclone_output', '.f90'),
    'preprocessed_psyclone',  # todo: this is no longer a collection, remove
    'configurator_output',
])

New version is now:

DEFAULT_SOURCE_GETTER = CollectionConcat([
    ArtefactSet.FORTRAN_BUILD_FILES,
    ArtefactSet.C_BUILD_FILES,
])

This also makes it MUCH easier to add a step (since there is no need to know the internals of Fab to figure out in which artefacts to find the required files). It also fixes (if I am not mistaken) the old approach where .f90 files are copied from src to build during preprocessing, BUT they are never added to an artefact, so the compilation will still be based on files in source.

hiker and others added 30 commits March 6, 2024 13:31
@hiker hiker marked this pull request as draft July 17, 2024 07:14
@hiker hiker added the enhancement New feature or request label Jul 17, 2024
@hiker hiker marked this pull request as ready for review July 19, 2024 14:00
@hiker
Copy link
Collaborator Author

hiker commented Jul 19, 2024

This is now ready. The PR looks so big because the change to the artefact names affected many files, plus I started to fix up files with re to coding standard like line lengths etc.

Copy link
Collaborator

@MatthewHambley MatthewHambley left a comment

Choose a reason for hiding this comment

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

This review turned into a real stream of consciousness so probably best to read all the comments before replying to any of them.

Looking at the proposed change, I'm not sure there's any value to continuing to support access by string. If it's only to provide backwards compatibility then I don't think we need to worry since the current release is 1.0 in name only.

source/fab/artefacts.py Outdated Show resolved Hide resolved
source/fab/artefacts.py Outdated Show resolved Hide resolved
source/fab/artefacts.py Show resolved Hide resolved
source/fab/artefacts.py Show resolved Hide resolved
source/fab/artefacts.py Show resolved Hide resolved
run_configs/lfric/lfric_common.py Outdated Show resolved Hide resolved
source/fab/parse/fortran.py Outdated Show resolved Hide resolved
source/fab/steps/archive_objects.py Show resolved Hide resolved
source/fab/steps/archive_objects.py Outdated Show resolved Hide resolved
tests/unit_tests/test_artefacts.py Outdated Show resolved Hide resolved
@hiker
Copy link
Collaborator Author

hiker commented Jul 23, 2024

Thanks for the review. I have renamed ALL_SOURCE->INITIAL_SOURCE as suggested, and opened #328 to find better names for other artefacts. As requested, I've closed the issues that I think are ok now. I'd suggest to leave the read-only-artefactstore for the telco. Not sure about the special handling of dictionary artefacts - open a ticket for later?

@hiker
Copy link
Collaborator Author

hiker commented Jul 23, 2024

Oh, I also reformatted some more files to be 80 characters or less (not all of them, some were rather huge, making it hard to see the important changes).

Copy link
Collaborator

@MatthewHambley MatthewHambley left a comment

Choose a reason for hiding this comment

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

I'm happy that conversations have been started around the things I spotted while reviewing this change. The change itself is a clear step forward and basis for future work.

@MatthewHambley MatthewHambley merged commit 015a024 into MetOffice:master Jul 25, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants