-
Notifications
You must be signed in to change notification settings - Fork 4
Support for Model output endpoint me.org #332
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
Changes from 9 commits
0b6e138
334e729
56b399c
52d2349
f411043
5305362
69c2f2a
11a5174
87dd853
fcf3a2c
b101645
b344ca3
57d53dc
45e1456
726d4e9
fc4c18b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,10 +6,14 @@ | |||||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import yaml | ||||||||||||||||||||||||||||||||||
| import copy | ||||||||||||||||||||||||||||||||||
| from cerberus import Validator | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import benchcab.utils as bu | ||||||||||||||||||||||||||||||||||
| from benchcab import internal | ||||||||||||||||||||||||||||||||||
| from benchcab.utils.repo import create_repo | ||||||||||||||||||||||||||||||||||
| from benchcab.model import Model | ||||||||||||||||||||||||||||||||||
| from typing import Optional | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| class ConfigValidationError(Exception): | ||||||||||||||||||||||||||||||||||
|
|
@@ -82,7 +86,7 @@ def read_optional_key(config: dict): | |||||||||||||||||||||||||||||||||
| Parameters | ||||||||||||||||||||||||||||||||||
| ---------- | ||||||||||||||||||||||||||||||||||
| config : dict | ||||||||||||||||||||||||||||||||||
| The configuration file with with/without optional keys | ||||||||||||||||||||||||||||||||||
| The configuration file with, or without optional keys | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||
| if "project" not in config: | ||||||||||||||||||||||||||||||||||
|
|
@@ -119,12 +123,84 @@ def read_optional_key(config: dict): | |||||||||||||||||||||||||||||||||
| config["fluxsite"]["pbs"] = internal.FLUXSITE_DEFAULT_PBS | config["fluxsite"].get( | ||||||||||||||||||||||||||||||||||
| "pbs", {} | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
| config["fluxsite"]["meorg_model_output_id"] = config["fluxsite"].get( | ||||||||||||||||||||||||||||||||||
| "meorg_model_output_id", internal.FLUXSITE_DEFAULT_MEORG_MODEL_OUTPUT_ID | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| config["codecov"] = config.get("codecov", False) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return config | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def is_valid_model_output_name(name: str) -> Optional[str]: | ||||||||||||||||||||||||||||||||||
| """Validate model output name against github issue standards. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Standard: <digit>-<words-sep-by-dashes> | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Parameters | ||||||||||||||||||||||||||||||||||
| ---------- | ||||||||||||||||||||||||||||||||||
| name: str | ||||||||||||||||||||||||||||||||||
| The model output name | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Returns | ||||||||||||||||||||||||||||||||||
| ------- | ||||||||||||||||||||||||||||||||||
| Optional[str] | ||||||||||||||||||||||||||||||||||
| If model output name does not meet standard, then return error message | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||
| if len(name) == 0: | ||||||||||||||||||||||||||||||||||
| return "Model output name is empty" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if len(name) > 255: | ||||||||||||||||||||||||||||||||||
| return "Model output name has length more than allowed limit on me.org (255)" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if " " in name: | ||||||||||||||||||||||||||||||||||
| return "Model output name cannot have spaces" | ||||||||||||||||||||||||||||||||||
|
abhaasgoyal marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| name_keywords = name.split("-") | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if not name_keywords[0].isdigit(): | ||||||||||||||||||||||||||||||||||
| return "Model output name does not start with number" | ||||||||||||||||||||||||||||||||||
|
abhaasgoyal marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if len(name_keywords) == 1: | ||||||||||||||||||||||||||||||||||
| return "Model output name does not contain keyword after number" | ||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm suggesting to change the error messages to be more direct, always using an affirmative sentence and importantly giving an example.
Suggested change
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 msgActually, the check on
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the check on |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def add_model_output_name(config: dict): | ||||||||||||||||||||||||||||||||||
| """Determine model output name from realisations. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Parameters | ||||||||||||||||||||||||||||||||||
| ---------- | ||||||||||||||||||||||||||||||||||
| config : dict | ||||||||||||||||||||||||||||||||||
| The configuration file with optional keys | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||
| # pure function | ||||||||||||||||||||||||||||||||||
| config = copy.deepcopy(config) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| is_model_output_name = False | ||||||||||||||||||||||||||||||||||
| for r in config["realisations"]: | ||||||||||||||||||||||||||||||||||
| assert not is_model_output_name | ||||||||||||||||||||||||||||||||||
| if r.pop("model_output_name", None): | ||||||||||||||||||||||||||||||||||
| is_model_output_name = True | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would revert to the previous implementation but using the
Suggested change
The implementation of
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To resolve this, I am using 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")).nameFor now, I'm not making it mandatory for |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| msg = is_valid_model_output_name(mo_name) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if msg is not None: | ||||||||||||||||||||||||||||||||||
| raise Exception(msg) | ||||||||||||||||||||||||||||||||||
|
SeanBryan51 marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| config["model_output_name"] = mo_name | ||||||||||||||||||||||||||||||||||
| break | ||||||||||||||||||||||||||||||||||
| assert is_model_output_name | ||||||||||||||||||||||||||||||||||
| return config | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def read_config_file(config_path: str) -> dict: | ||||||||||||||||||||||||||||||||||
| """Load the config file in a dict. | ||||||||||||||||||||||||||||||||||
|
|
@@ -154,6 +230,8 @@ def read_config(config_path: str) -> dict: | |||||||||||||||||||||||||||||||||
| ---------- | ||||||||||||||||||||||||||||||||||
| config_path : str | ||||||||||||||||||||||||||||||||||
| Path to the configuration file. | ||||||||||||||||||||||||||||||||||
| is_meorg: str | ||||||||||||||||||||||||||||||||||
| Whether workflow includes meorg job submission. If true, determine the model output name | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Returns | ||||||||||||||||||||||||||||||||||
| ------- | ||||||||||||||||||||||||||||||||||
|
|
@@ -169,7 +247,8 @@ def read_config(config_path: str) -> dict: | |||||||||||||||||||||||||||||||||
| # Read configuration file | ||||||||||||||||||||||||||||||||||
| config = read_config_file(config_path) | ||||||||||||||||||||||||||||||||||
| # Populate configuration dict with optional keys | ||||||||||||||||||||||||||||||||||
| read_optional_key(config) | ||||||||||||||||||||||||||||||||||
| # Validate and return. | ||||||||||||||||||||||||||||||||||
| config = read_optional_key(config) | ||||||||||||||||||||||||||||||||||
| # Validate. | ||||||||||||||||||||||||||||||||||
| validate_config(config) | ||||||||||||||||||||||||||||||||||
| config = add_model_output_name(config) | ||||||||||||||||||||||||||||||||||
| return config | ||||||||||||||||||||||||||||||||||
|
abhaasgoyal marked this conversation as resolved.
|
Uh oh!
There was an error while loading. Please reload this page.