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

Linter: Internalize functionality and scheduler integration stub #44

Merged
merged 23 commits into from
Mar 31, 2023

Conversation

reuterbal
Copy link
Collaborator

Note: filing this early for test coverage and while testing offline.

This PR moves some of the functionality from loki-lint.py into the loki.lint module to facilitate testing. In that process, some of the functionality could be replaced by internal utility functions. With this, we now have a new entry point for linting: lint_files

This PR also bundles a first stub of a scheduler integration for the linter. It implements linting as a transformation that is applied across the scheduler's graph.
However, since linting is file based, this required equipping the Scheduler with a file-centric view of dependencies. For that purpose, the scheduler has been equipped with the ability to generate a file graph for the scheduler's item_graph, and optionally use that in process. This may be handy for the FileWriteTransformation, too.

The CLI script for checking in loki-lint.py is now only responsible for reading the YAML config and filling the config dict.

@github-actions
Copy link

github-actions bot commented Mar 8, 2023

Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/44

@codecov-commenter
Copy link

codecov-commenter commented Mar 9, 2023

Codecov Report

Merging #44 (2f328e5) into main (13ab01d) will increase coverage by 0.57%.
The diff coverage is 90.99%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #44      +/-   ##
==========================================
+ Coverage   91.07%   91.65%   +0.57%     
==========================================
  Files          86       86              
  Lines       15043    15206     +163     
==========================================
+ Hits        13701    13937     +236     
+ Misses       1342     1269      -73     
Flag Coverage Δ
lint_rules 97.36% <ø> (+<0.01%) ⬆️
loki 91.93% <90.99%> (+0.61%) ⬆️
transformations 86.17% <ø> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
loki/frontend/fparser.py 92.90% <0.00%> (ø)
loki/tools/files.py 79.03% <ø> (+9.67%) ⬆️
loki/lint/utils.py 74.44% <64.28%> (+25.14%) ⬆️
loki/bulk/scheduler.py 77.95% <93.87%> (+2.58%) ⬆️
loki/lint/reporter.py 89.69% <97.43%> (+27.56%) ⬆️
loki/lint/linter.py 96.20% <97.70%> (+14.49%) ⬆️
loki/tools/util.py 77.15% <100.00%> (+0.09%) ⬆️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@reuterbal reuterbal marked this pull request as ready for review March 9, 2023 20:29
@reuterbal reuterbal requested a review from mlange05 March 9, 2023 20:29
@reuterbal
Copy link
Collaborator Author

I believe this is ready for a look.

Copy link
Collaborator

@mlange05 mlange05 left a comment

Choose a reason for hiding this comment

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

Looks great, neat and tidy as always. Especially the Scheduler changes to traversal and the file graph are much appreciated. CI and regression tests are looking good, so GTG from me.

nx.DiGraph
"""
paths = {item.path for item in self.item_graph}
basepath = Path(commonpath([str(p) for p in paths]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

[no action] Neat. Might need to use this approach in Scheduler in general.

@mlange05 mlange05 merged commit 4dd6fc8 into main Mar 31, 2023
@mlange05 mlange05 deleted the nabr-linter-via-scheduler branch March 31, 2023 05:06
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.

3 participants