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

Added command line option to lint the configuration setup #5249

Merged
merged 41 commits into from
Apr 25, 2023

Conversation

yngve-sk
Copy link
Contributor

@yngve-sk yngve-sk commented Apr 18, 2023

Issue
Resolves #5248

Approach
Added command line option ert lint which just produces errors and warnings from the configuration file:

Screenshot 2023-04-19 at 13 17 45

Pre review checklist

  • Added appropriate release note label
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Updated documentation
  • Ensured new behaviour is tested

Adding labels helps the maintainers when writing release notes. This is the list of release note labels.

@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2023

Codecov Report

Merging #5249 (fe69957) into main (abd5035) will increase coverage by 0.03%.
The diff coverage is 82.01%.

@@            Coverage Diff             @@
##             main    #5249      +/-   ##
==========================================
+ Coverage   73.20%   73.23%   +0.03%     
==========================================
  Files         391      395       +4     
  Lines       26946    27100     +154     
  Branches     1965     1965              
==========================================
+ Hits        19725    19846     +121     
- Misses       6657     6690      +33     
  Partials      564      564              
Impacted Files Coverage Δ
src/ert/parsing/config_linting.py 0.00% <0.00%> (ø)
src/ert/__main__.py 78.73% <50.00%> (-0.20%) ⬇️
src/ert/parsing/error_info.py 71.42% <71.42%> (ø)
src/ert/parsing/context_values.py 80.39% <80.39%> (ø)
src/ert/parsing/config_errors.py 88.88% <89.47%> (-11.12%) ⬇️
src/ert/_c_wrappers/config/config_parser.py 94.59% <100.00%> (+0.07%) ⬆️
src/ert/_c_wrappers/enkf/enkf_obs.py 87.50% <100.00%> (+0.12%) ⬆️
src/ert/_c_wrappers/enkf/ensemble_config.py 96.23% <100.00%> (+0.27%) ⬆️
src/ert/parsing/config_keywords.py 99.33% <100.00%> (+0.04%) ⬆️
src/ert/parsing/file_context_token.py 100.00% <100.00%> (ø)
... and 2 more

... and 18 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@yngve-sk yngve-sk force-pushed the detailed-error-collection-p1 branch 2 times, most recently from b3435d5 to 0f457a7 Compare April 18, 2023 13:47
@yngve-sk yngve-sk force-pushed the detailed-error-collection-p1 branch from 0f457a7 to efd4a45 Compare April 19, 2023 05:40
@yngve-sk yngve-sk force-pushed the detailed-error-collection-p1 branch 2 times, most recently from cadc1e2 to efa62f7 Compare April 19, 2023 06:05
@yngve-sk yngve-sk marked this pull request as ready for review April 19, 2023 06:07
@yngve-sk yngve-sk changed the title Detailed error collection initials Detailed error collection + initial cli linting setup Apr 19, 2023
@yngve-sk yngve-sk force-pushed the detailed-error-collection-p1 branch from efa62f7 to cf585bc Compare April 19, 2023 06:12
@yngve-sk yngve-sk force-pushed the detailed-error-collection-p1 branch from cf585bc to 83f0112 Compare April 19, 2023 06:13
src/ert/__main__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@eivindjahren eivindjahren left a comment

Choose a reason for hiding this comment

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

This is a good direction. I commented on some smaller issues. Generally I would like to see more tests as there is a lot of new functionality here. As for the general approach to the parser module, a second opinion by @mortalisk would be good.

@yngve-sk yngve-sk requested a review from mortalisk April 19, 2023 08:00
@yngve-sk yngve-sk force-pushed the detailed-error-collection-p1 branch from 8d30307 to 324e2f9 Compare April 19, 2023 10:52
args: List[Any],
keyword: FileContextToken,
) -> Union[List[Any], Any]:
errors = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to annotate this as errors: List[Union[ErrorInfo, ConfigValidationError]] = []

)
if self.argc_max == 1 and self.argc_min == 1:

elif self.argc_max == 1 and self.argc_min == 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

This could return args[0] with an error in the errors list coming from token_to_value_with_context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in case here it makes sense to check for errors before doing the return statement

@mortalisk
Copy link
Contributor

by setting "ert" as the value to NUM_REALIZATIONS I got this. a repetition of the same error.

  * 'NUM_REALIZATIONS' must have an integer value as argument 1 at line 11, column 18-21
  * NUM_REALIZATIONS must have at least 1 arguments at line 11, column 1-17;Parsing config file `poly.ert` resulted in the errors: 
  * NUM_REALIZATIONS must be set. at line None, column None-None
  * NUM_REALIZATIONS must be set. at line None, column None-None

Yngve S. Kristiansen added 2 commits April 24, 2023 15:46
@yngve-sk
Copy link
Contributor Author

yngve-sk commented Apr 25, 2023

by setting "ert" as the value to NUM_REALIZATIONS I got this. a repetition of the same error.

  * 'NUM_REALIZATIONS' must have an integer value as argument 1 at line 11, column 18-21
  * NUM_REALIZATIONS must have at least 1 arguments at line 11, column 1-17;Parsing config file `poly.ert` resulted in the errors: 
  * NUM_REALIZATIONS must be set. at line None, column None-None
  * NUM_REALIZATIONS must be set. at line None, column None-None

Fixed w/ test & some change to control flow, it happened due to 2 issues:

(1) if a kw like NUM_REALIZATIONS is specified to "ert", it is now not added to the config_dict as it is invalid, then check_required does not find it in the config_dict and also flags it as undeclared which is not the case. Fix: have check_required take in a Set[str] of declared values, because it checks if something is declared, not if it is valid & parsed.

(2) the alias NUM_REALISATIONS / NUM_REALIZATIONS caused a duplicate check in check_required, due to this:

    ...
    for constraints in schema.values():
    ....

schema.values() returns the NUM_REALIZATIONS entry twice.
Fix: Visit each kw only once:

    visited: Set[str] = set()

    for constraints in schema.values():
        if constraints.kw in visited:
            continue

@yngve-sk
Copy link
Contributor Author

yngve-sk commented Apr 25, 2023

As for use of __new__ instead of __init__ when overriding int, str, float, which are immutable classes, there was a reason:

new() is intended mainly to allow subclasses of immutable types (like int, str, or tuple) to customize instance creation. It is also commonly overridden in custom metaclasses in order to customize class creation.

So a subclass overriding the init of its immutable superclass will give an error when initialized with more arguments than that of its superclass.

For ContextBool it works because it is a hacky way of emulating bool behavior with __bool__, as it is not possible to inherit from the bool class.

specifically: NUM_REALIZATIONS in check_required
@yngve-sk yngve-sk force-pushed the detailed-error-collection-p1 branch 2 times, most recently from 151cb9a to 80a9807 Compare April 25, 2023 07:07
@eivindjahren eivindjahren added the release-notes:new-feature Automatically categorise as new feature in release notes label Apr 25, 2023
@eivindjahren eivindjahren changed the title Detailed error collection + initial cli linting setup Added command line option to lint the configuration setup Apr 25, 2023
token: FileContextToken
keyword_token: FileContextToken

def __init__(self, val: bool, token: str, keyword_token: FileContextToken):
Copy link
Contributor

Choose a reason for hiding this comment

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

Mypy complains about a mismatch of types in self.token = token

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, should be FileContextToken, fixed

@yngve-sk yngve-sk force-pushed the detailed-error-collection-p1 branch from 80a9807 to 59e484c Compare April 25, 2023 07:20
@mortalisk
Copy link
Contributor

Remember to squash, and tick all the boxes

@yngve-sk yngve-sk force-pushed the detailed-error-collection-p1 branch 2 times, most recently from 62f15bb to 0db17a0 Compare April 25, 2023 11:32
@yngve-sk yngve-sk force-pushed the detailed-error-collection-p1 branch from 0db17a0 to fe69957 Compare April 25, 2023 11:33
Copy link
Contributor

@eivindjahren eivindjahren left a comment

Choose a reason for hiding this comment

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

Remember to squash and merge

@yngve-sk yngve-sk merged commit 3844ff8 into equinor:main Apr 25, 2023
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:new-feature Automatically categorise as new feature in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up minimal linting + basic building blocks for detailed error collection
4 participants