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.

Sign up for GitHub

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

Jump to bottom

Fix the import of the new industry module #386

Merged
merged 3 commits into from
Jun 6, 2024
Merged

Fix the import of the new industry module #386

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb3c2f7
Fix industry module import
timtroendle Jun 1, 2024
8e57480
Update schema CI test to allow for validating parts of config
timtroendle Jun 1, 2024
5df2a6d
Update validation of module config to happen within module
timtroendle Jun 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/schemavalidation.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,4 @@ jobs:
run: python ./tests/validate_schema.py ./tests/resources/schema.yaml
# Modules
- name: Validate industry config
run: python ./tests/validate_schema.py ./modules/industry/schema.yaml --config ./modules/industry/config.yaml
run: python ./tests/validate_schema.py ./modules/industry/schema.yaml --config ./modules/industry/config.yaml --level industry
14 changes: 2 additions & 12 deletions Snakefile
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

The core bug is fixed by adding default_target: True to rule all:

This should avoid having to heavily modify the industry module beyond the schema improvements.

Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,6 @@ from snakemake.utils import validate, min_version, makedirs
configfile: "config/default.yaml"
validate(config, "config/schema.yaml")

# >>>>>> Include modules >>>>>>
# Industry
configfile: "modules/industry/config.yaml"
validate(config, "modules/industry/schema.yaml")

module module_industry:
snakefile: "modules/industry/industry.smk"
config: config["industry"]
use rule * from module_industry as module_industry_*
# <<<<<< Include modules <<<<<<


root_dir = config["root-directory"] + "/" if config["root-directory"] not in ["", "."] else ""
__version__ = open(f"{root_dir}VERSION").readlines()[0].strip()
test_dir = f"{root_dir}tests/"
Expand All @@ -38,6 +26,7 @@ include: "./rules/nuclear.smk"
include: "./rules/transport.smk"
include: "./rules/sync.smk"
include: "./rules/heat.smk"
include: "./rules/modules.smk"
min_version("8.10")
localrules: all, clean
wildcard_constraints:
Expand Down Expand Up @@ -83,6 +72,7 @@ onerror:
rule all:
message: "Generate euro-calliope pre-built models and run tests."
localrule: True
default_target: True
input:
"build/logs/continental/test.success",
"build/logs/national/test.success",
Expand Down
87 changes: 41 additions & 46 deletions modules/industry/schema.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,55 +1,50 @@
$schema: https://json-schema.org/draft/2020-12/schema
type: object
additionalProperties: true
additionalProperties: false
properties:
industry:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above. The definition of this level is crucial for modules to work properly.

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to remove it from here (and necessary for schema validation given that the config is merged with the rest of the project config). validating would become validate(config["industry"], "../modules/industry/schema.yaml") which means industry: should not be in the schema.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's important to have it here so that the module makes sure you are specifying the module configuration separately. Otherwise, someone might add all these at the top level, causing conflicts!

The only limitation that this might impose is that modules cannot expect the same name, which is good, imo.

description: Module subsection to ensure no name conflicts are possible between modules.
inputs:
type: object
additionalProperties: false
description: Inputs are paths of prerequired files.
properties:
inputs:
type: object
additionalProperties: false
description: Inputs are paths of prerequired files.
properties:
path-energy-balances:
type: string
description: |
Annual energy balance file.
Columns [cat_code,carrier_code,unit,country,year,value].
path-cat-names:
type: string
description: |
Category mapping file.
Columns [cat_code,top_cat,sub_cat_contribution,sub_cat_1,sub_cat_2,jrc_idees].
path-carrier-names:
type: string
description: |
Carrier mapping file.
Columns [carrier_code,carrier_name,hh_carrier_name,com_carrier_name,ind_carrier_name,oth_carrier_name].
path-jrc-industry-energy:
type: string
description: |
JRC processed industry energy demand .nc file.
path-jrc-industry-production:
type: string
description: |
JRC processed industrial production .nc file.
outputs:
type: object
description: Outputs are paths for the files produced by the module.
params:
path-energy-balances:
type: string
description: |
Annual energy balance file.
Columns [cat_code,carrier_code,unit,country,year,value].
path-cat-names:
type: string
description: |
Category mapping file.
Columns [cat_code,top_cat,sub_cat_contribution,sub_cat_1,sub_cat_2,jrc_idees].
path-carrier-names:
type: string
description: |
Carrier mapping file.
Columns [carrier_code,carrier_name,hh_carrier_name,com_carrier_name,ind_carrier_name,oth_carrier_name].
path-jrc-industry-energy:
type: string
description: |
JRC processed industry energy demand .nc file.
path-jrc-industry-production:
type: string
description: |
JRC processed industrial production .nc file.
outputs:
type: object
description: Outputs are paths for the files produced by the module.
params:
type: object
additionalProperties: false
description: Parameters allow users to configure module behaviour.
properties:
steel:
type: object
additionalProperties: false
description: Parameters allow users to configure module behaviour.
description: "Parameters specific to the 'Iron and steel' industry category."
properties:
steel:
type: object
additionalProperties: false
description: "Parameters specific to the 'Iron and steel' industry category."
properties:
recycled-steel-share:
type: number
description: "Share of recycled metal in the H-DRI steel process."
minimum: 0
maximum: 1
recycled-steel-share:
type: number
description: "Share of recycled metal in the H-DRI steel process."
minimum: 0
maximum: 1
9 changes: 9 additions & 0 deletions rules/modules.smk
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Industry
configfile: "./modules/industry/config.yaml"
validate(config["industry"], "../modules/industry/schema.yaml")
Copy link
Contributor

Choose a reason for hiding this comment

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

Just one final improvement.

I think we should move this validation within the module's smk. This way, we avoid users skipping validation because they find it cumbersome and always enforce it.

That way, we still force users to separate a module's configuration due to additionalProperties: false (although it's not really explicit code-wise).

Could you test this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense to me, also in terms of separation of concerns. I tested it and it works.



module module_industry:
snakefile: "../modules/industry/industry.smk"
config: config["industry"]
use rule * from module_industry as module_industry_*
9 changes: 9 additions & 0 deletions tests/validate_schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@
"If not given, schema itself will be validated against JSON schema Draft 2020-12"
),
)
parser.add_argument(
"--level",
help=(
"level of the configuration to validate. "
"If not given, this will validate the entire configuration."
),
)
args = parser.parse_args()

with open(args.schema) as f:
Expand All @@ -20,6 +27,8 @@
if args.config:
with open(args.config) as f:
config = yaml.safe_load(f)
if args.level:
config = config[args.level]
jsonschema.validate(config, schema)
else:
jsonschema.Draft202012Validator.check_schema(schema)
Loading