Support for Model output endpoint me.org #332

abhaasgoyal · 2025-07-01T23:28:50Z

Resolves #331

Create a workflow for benchcab not requiring for the user to enter model_output_id anymore. Instead, the user needs to specify which branch should be chosen as the model_output_name, from which benchcab does the necessary workflow.

Uses the new API endpoints from me.org API v3.0.0 (see CABLE-LSM/meorg_client#67 for more details)

codecov · 2025-08-17T22:29:34Z

Codecov Report

❌ Patch coverage is 79.68750% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.60%. Comparing base (8e0d58f) to head (fc4c18b).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
src/benchcab/utils/meorg.py	27.27%	8 Missing ⚠️
src/benchcab/config.py	93.87%	3 Missing ⚠️
src/benchcab/benchcab.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #332      +/-   ##
==========================================
+ Coverage   68.28%   69.60%   +1.32%     
==========================================
  Files          21       21              
  Lines        1157     1214      +57     
==========================================
+ Hits          790      845      +55     
- Misses        367      369       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ccarouge

Since Sean is away, I've started looking at the PR. I haven't reacquainted myself completely with benchcab so for the moment most of my comments are not much on the structure. I'll need more time for that to come.

src/benchcab/data/meorg_jobscript.j2

ccarouge · 2025-08-20T05:21:01Z

src/benchcab/data/meorg_jobscript.j2

+if [ ! -z "${MODEL_OUTPUT_ID}" ] ; then
+echo "Deleting existing files from model output ID"
+$MEORG_BIN file delete_all $MODEL_OUTPUT_ID
+echo "Updated model output ID"


I don't understand why we need this output here.

If a model output ID already exists with a given name, we want to preserve the model output ID (since in case we assume that the user is re-running the same experiment). However, we want to clean up any existing files, so that the experiment runs with the intended files of the user. (I have added a short comment on the same)

ccarouge · 2025-08-20T05:22:12Z

src/benchcab/data/meorg_jobscript.j2

 sleep $CACHE_DELAY

+{% for exp_id in model_exp_ids %} 
+echo "Replace benchmarks to model output"


Using "Add benchmarks ..." in the output would make more sense to me than "Replace"

I have updated the comment to "Add". The reason I chose "Replace" was, unlike adding experiments, if a benchmark already exists and we use meorg benchmark update, the existing benchmarks get overwritten (but yeah I don't see a use case for the user to not add benchmarks from scratch everytime in this workflow).

src/benchcab/data/meorg_jobscript.j2

ccarouge · 2025-08-20T05:26:52Z

src/benchcab/config.py

    ----------
    config : dict
-        The configuration file with with/without optional keys
+        The configuration file with without optional keys


Neither "with without optional keys" nor "with/without optional keys" make any sense. Any idea what we are trying to say here?

I believe it's like the user may already have passed in the optional key (thus not replacing it), otherwise if the config file is without the necessary optional key, it would be replaced by a value calculated in read_optional_key.

src/benchcab/config.py

src/benchcab/internal.py

ccarouge · 2025-08-20T06:02:43Z

tests/test_config.py

        "modules": ["intel-compiler/2021.1.1", "netcdf/4.7.4", "openmpi/4.1.0"],
        "realisations": [
-            {"repo": {"svn": {"branch_path": "trunk"}}},
+            {"repo": {"svn": {"branch_path": "trunk"}}, "model_output_name": True},


Ideally, this should return an error because just using the name "trunk" for the model output name should not be allowed.

I have added in the validation function for checking model output name derived from config file, along with tests.

src/benchcab/data/test/config-basic.yml

ccarouge

Second batch of comments. It tends to come bits by bits sorry.

I believe the benchcab's User Guide needs some modifications which are missing from this PR.

ccarouge · 2025-08-25T04:35:19Z

src/benchcab/config.py

+            mo_name = None
+            if r.get("name"):
+                mo_name = r["name"]
+            else:
+                repo = create_repo(
+                    spec=r["repo"],
+                    path=internal.SRC_DIR / (r["name"] if r.get("name") else Path()),
+                )
+                mo_name = Model(repo).name


I would revert to the previous implementation but using the name argument in create_repo()

Suggested change

mo_name = None

if r.get("name"):

mo_name = r["name"]

else:

repo = create_repo(

spec=r["repo"],

path=internal.SRC_DIR / (r["name"] if r.get("name") else Path()),

)

mo_name = Model(repo).name

mo_name = None

repo = create_repo(

spec=r["repo"],

path=internal.SRC_DIR / (r["name"] if r.get("name") else Path()),

name=r.get("name"),

)

mo_name = Model(repo).name

The implementation of Model().__init__() should assign to Model(repo).name:

the branch name if name is not present in the config

the value of name if it's present.

To resolve this, I am using r.get("name") both in Repo and Model initialisations since Model cannot determine the name just from Repo (unless it has a mandatory name parameter, which it doesn't for all the sub classes - for example LocalRepo has name but GitRepo doesn't)

repo = create_repo( spec=r["repo"], path=internal.SRC_DIR / (r["name"] if r.get("name") else Path()), ) mo_name = Model(repo, name=r.get("name")).name

For now, I'm not making it mandatory for Repo to have a name parameter (so usage of r.get("name") twice is fine.

ccarouge · 2025-08-25T04:39:09Z

src/benchcab/config.py

+        return "Model output name does not start with number"
+
+    if len(name_keywords) == 1:
+        return "Model output name does not contain keyword after number"


I'm suggesting to change the error messages to be more direct, always using an affirmative sentence and importantly giving an example.

Suggested change

return "Model output name does not contain keyword after number"

return "Model output name must contain keywords after the initial number. E.g. 123-fixing-met-file-reading-error"

I also wonder if we want to build an error message that indicates all the mistakes in a given name at once, instead of returning on the first error encountered. Something like the following (incomplete and not formatted for the code):

msg="" if len(name) == 0: msg+="Model output name can not be empty.\n" if len(name) > 255: msg+="The length of model output name must be shorter than 255 characters.\n" if msg: msg+="Example valid name: 123-fixing-met-file-reading-error" return msg

Actually, the check on len(name)==0 indicates a problem with the code itself rather than the config I suspect so it might be weird to give a valid name example but I think it's a small inconvenience.

For the check on len(name)==0, I'm trying to treat the function as independent entity from the code (so if the code does have issues in the future, then it would be caught more easily). Let me know if this check is still not needed.

src/benchcab/config.py

tests/test_config.py

docs/user_guide/config_options.md

ccarouge · 2025-09-02T01:28:20Z

@abhaasgoyal I won't have much time to continue the review. I'll let @SeanBryan51 have a look. Sorry for switching reviewer all the time!

SeanBryan51 · 2025-09-03T00:00:34Z

Hi @abhaasgoyal, as I understand these changes require CABLE-LSM/meorg_client#67. Can we merge that PR first and push a new meorg release so that in this PR we can update the minimum meorg version in meta.yaml and benchcab-dev.yaml?

abhaasgoyal · 2025-09-03T00:49:34Z

@SeanBryan51, yes happy to get that merged in first before merging this

SeanBryan51 · 2025-09-16T00:42:12Z

Hi @abhaasgoyal, now that CABLE-LSM/meorg_client#67 is merged and we have a new 0.5.0 release, please update the minimum meorg version in meta.yaml and benchcab-dev.yaml

src/benchcab/benchcab.py

docs/user_guide/config_options.md

tests/test_config.py

docs/user_guide/config_options.md

src/benchcab/config.py

abhaasgoyal · 2025-09-24T02:40:59Z

About design decision recommendations for determining model output name:

Concatenation of realisations: On discussion, the workflow for most users is picking up specific branches as the name even if they have multiple realisations. They also want to minimise length of model output name for clarity. So the current methodology imposed seems to be the most suitable. It also does not restrict flexibility, since the user can edit the model output name via the name parameter as well.
TODO: A small part of hash [f(model_output_name, user, profile)], to go with model output name (upto 6 characters) at the end, with _ as the separator. For example - 123-my-branch_38j4ka

SeanBryan51 · 2025-09-24T06:20:42Z

Hi @abhaasgoyal, thanks for the update.

In the meeting I suggested checking the issue number (inferred from meorg_output_name) actually pointed to a valid issue on GitHub. If this seems a bit overkill for the amount of effort necessary to achieve this, I'm happy to leave this as is and have this go in in a later PR.

Apologies for being pedantic about these things

abhaasgoyal · 2025-10-01T03:14:26Z

I discussed with Gab about the hash structure for meorg_output_name. Some observations as follows:

The information for the user is not currently available in benchcab (it's in a .json file accessible by meorg_client).
It would be nice to have a unique hash related to all realisations, rather than just the one branch.

So for now, the proposed hash is f(realisation_names, profile). Regarding point 1, if we want to add the user in the hash as well, it can happen via importing the meorg_client package in benchcab. Is it okay to mix CLI calls and using internal package functions as well.

@SeanBryan51 I'll create an separate issue for validating meorg_output_name w.r.t. github issues

SeanBryan51

Looks good! Just a small english rewording suggestion for the doc.

docs/user_guide/config_options.md

Co-authored-by: Sean Bryan <39685865+SeanBryan51@users.noreply.github.com>

abhaasgoyal added 2 commits June 18, 2025 13:12

define workflow

0b6e138

Merge branch 'main' into 331-model-output-endpoint-meorg

334e729

abhaasgoyal marked this pull request as draft July 1, 2025 23:29

abhaasgoyal added 3 commits August 6, 2025 12:15

working script ncitest

56b399c

set config option for name (coupled for now)

52d2349

Add functionality for config model output name

f411043

abhaasgoyal added 3 commits August 18, 2025 08:57

modifications for pr compatibility

5305362

fix au-tum id

69c2f2a

reset default experiment to 42 site test

11a5174

abhaasgoyal requested review from a team and SeanBryan51 and removed request for a team August 17, 2025 23:11

abhaasgoyal marked this pull request as ready for review August 17, 2025 23:11

ccarouge requested changes Aug 20, 2025

View reviewed changes

resolve pr issues

87dd853

ccarouge requested changes Aug 25, 2025

View reviewed changes

abhaasgoyal added 2 commits August 27, 2025 12:20

pr issue resolve

fcf3a2c

update

b101645

check name length

b344ca3

remove old tests for meorg_output_name

57d53dc

Update yml versions

45e1456