Skip to content

Clean parsing of optional config.yaml data#238

Merged
abhaasgoyal merged 18 commits intomainfrom
234-clean-parsing-optional
Jan 25, 2024
Merged

Clean parsing of optional config.yaml data#238
abhaasgoyal merged 18 commits intomainfrom
234-clean-parsing-optional

Conversation

@abhaasgoyal
Copy link
Copy Markdown

@abhaasgoyal abhaasgoyal commented Jan 19, 2024

Fixes #234, #239

Merge Message

  • Parse config.yaml and fill all optional data before validation. We assume that using keys in config would never lead to KeyError in other modules after validation.
  • Correction for validating config schema:
    • Change multiprocessing to multiprocess to match documentation
    • Regex match forwalltime to match single digit numbers
    • Remove Deprecation warning for (?i) flag at the start for mem

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 21, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (d1d6280) 58.22% compared to head (2efcd48) 59.13%.
Report is 1 commits behind head on main.

Files Patch % Lines
benchcab/benchcab.py 0.00% 4 Missing ⚠️
tests/test_config.py 97.36% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #238      +/-   ##
==========================================
+ Coverage   58.22%   59.13%   +0.90%     
==========================================
  Files          29       29              
  Lines        2109     2129      +20     
==========================================
+ Hits         1228     1259      +31     
+ Misses        881      870      -11     

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

@abhaasgoyal abhaasgoyal force-pushed the 234-clean-parsing-optional branch from c255bf3 to 39049bd Compare January 21, 2024 05:16
@ccarouge ccarouge self-assigned this Jan 22, 2024
@abhaasgoyal abhaasgoyal changed the title DRAFT: Clean parsing of optional config.yaml data Clean parsing of optional config.yaml data Jan 22, 2024
@abhaasgoyal abhaasgoyal requested a review from ccarouge January 22, 2024 23:06
Copy link
Copy Markdown
Member

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

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

A couple of minor changes and some documentation/linting issues to solve.
Re-ask for a review via GitHub when you are done.

Comment thread benchcab/config.py
Comment thread benchcab/config.py Outdated
Comment thread benchcab/data/config-schema.yml
Comment thread benchcab/benchcab.py Outdated
Comment thread benchcab/config.py
Copy link
Copy Markdown
Member

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

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

A few things I missed. And where are the tests for read_optional_data?

Comment thread benchcab/utils/pbs.py Outdated
Comment thread benchcab/config.py
Comment thread tests/test_pbs.py Outdated
@ccarouge
Copy link
Copy Markdown
Member

Actually, can we rename read_optional_data? I don't like the use of data here. In the doc, we are using key for the entries in the config file maybe that's the best choice.

@abhaasgoyal abhaasgoyal requested a review from ccarouge January 24, 2024 01:56
@abhaasgoyal abhaasgoyal force-pushed the 234-clean-parsing-optional branch from 8d37402 to 04c86bb Compare January 24, 2024 02:55
@abhaasgoyal abhaasgoyal force-pushed the 234-clean-parsing-optional branch from 04c86bb to b1514a5 Compare January 24, 2024 02:55
Comment thread tests/test_pbs.py
Copy link
Copy Markdown
Member

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

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

It looks good. I'm requesting changes because of the fixtures for the tests but the other comments are minor.

Comment thread benchcab/internal.py Outdated
Comment thread benchcab/utils/pbs.py Outdated
Comment thread tests/test_config.py
@abhaasgoyal abhaasgoyal force-pushed the 234-clean-parsing-optional branch from 1cad0c1 to 5b6c6fa Compare January 25, 2024 00:29
@abhaasgoyal abhaasgoyal requested a review from ccarouge January 25, 2024 00:56
Comment thread docs/user_guide/index.md Outdated
Copy link
Copy Markdown
Member

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

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

All good. A couple of simple suggestions.

Comment thread tests/test_config.py Outdated
Comment thread tests/test_config.py Outdated
@abhaasgoyal abhaasgoyal force-pushed the 234-clean-parsing-optional branch from 90feba8 to 2efcd48 Compare January 25, 2024 01:37
@abhaasgoyal abhaasgoyal merged commit b7227bb into main Jan 25, 2024
@abhaasgoyal abhaasgoyal deleted the 234-clean-parsing-optional branch January 25, 2024 02:31
@abhaasgoyal abhaasgoyal linked an issue Jan 29, 2024 that may be closed by this pull request
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.

Correct config schema for multiprocess and walltime Clean parsing of optional config.yaml data

2 participants