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

Add profiling utilities #3

Merged
merged 6 commits into from
May 14, 2024
Merged

Add profiling utilities #3

merged 6 commits into from
May 14, 2024

Conversation

micaeljtoliveira
Copy link
Collaborator

@micaeljtoliveira micaeljtoliveira commented Feb 21, 2024

Add utilities to handle FMS and ESMF profiling information. These allow to easily generate plots for scaling studies.

Copy link

codecov bot commented Feb 21, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

… the tests directory to test_utils to avoid nameclashes.
@micaeljtoliveira
Copy link
Collaborator Author

@dougiesquire Would you have time to look at this PR? It's quite large, but I would be more interested in general comments about structure and design, rather than a careful review of all the details.

Copy link
Collaborator

@dougiesquire dougiesquire left a comment

Choose a reason for hiding this comment

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

This looks great @micaeljtoliveira. My first thought is that these tools are sufficiently general and useful beyond OM3 to justify moving them into their own package. Something to do now, later or never?

Since you asked for it, here's an attempt at a proposed restructure that I think makes things clearer for users and developers. Of course, this is all a matter of opinion so feel free to ignore all or parts of it:

om3utils/
├── __init__.py
├── config_file
│   ├── __init__.py # import all read_*.py and write_*.py functions
│   ├── mom6_input.py
│   ├── nuopc_config.py
│   └── payu_config_yaml.py
├── profiling
│   ├── __init__.py
│   ├── parser
│   │   ├── __init__.py # import ESMFProfilingParser and FMSProfilingParser
│   │   ├── base.py # contains ProfilingParser from profiling.py
│   │   ├── esmf.py # contains all of esmf_profiling.py and esmf_trace.py
│   │   └── fms.py # contains all of fms_profiling.py
│   └── analysis.py # contains all of profiling_analyses.py and parse_profiling_data from profiling.py
└── utils.py

Then the profiling tools could be used as follows:

from om3utils.profiling.parser import ESMFProfilingParser
from om3utils.profiling.analysis import (
    parse_profiling_data, 
    scaling_efficiency
)

stats = parse_profiling_data(
    run_dirs,
    ESMFProfilingParser,
    varname,
    getvar
)
eff = scaling_efficiency(stats)

You could even import all the parsers and analysis functions in profiling.__init__.py so that one does not need to go through the parser and analysis subpackages.

(Note I'm not really sold on the name of the config_file subpackage)

@micaeljtoliveira
Copy link
Collaborator Author

@dougiesquire Thanks for the feedback, lots of good points. I now realize that this PR is probably not the best place to discuss these, as they are quite broad and general. I'll open an issue for this. In order not to stall development anymore, I'll merge this branch as is, unless you want to have a more detailed look.

@dougiesquire
Copy link
Collaborator

Happy to merge 🙂

@micaeljtoliveira micaeljtoliveira merged commit 286a3d7 into main May 14, 2024
4 of 6 checks passed
@micaeljtoliveira micaeljtoliveira deleted the profiling branch May 14, 2024 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants