You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/01/07 08:54:52 UTC

[GitHub] [airflow] potiuk opened a new issue #20740: Breeze: Running static checks with Breeze

potiuk opened a new issue #20740:
URL: https://github.com/apache/airflow/issues/20740


   We should rewrite the action to run static checks with Breeze. Currently implemented by 'static-checks' opetoin. 
   
   The should baiscally run appropriate `pre-commit run` statement. The difference vs. running just pre-commit is that it should implement auto-complete of checks available (pre-commit does not have it) and make sure that CI image is built for the checks that require it. The list of available static checks should be retrieved by parsing the .pre-commit.yml file rather than (as it is currently done) maintaining the list in ./breeze-complete script
   
   More info on static checks: https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Bowrna commented on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

Bowrna commented on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1010809006


   ` The list of available static checks should be retrieved by parsing the .pre-commit.yml file rather than (as it is currently done) maintaining the list in ./breeze-complete script`
   
   @potiuk  Do you think we have to parse the .pre-commit.yml  file on the fly and enable auto-complete for those checks rather than adding click options ?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1010978696

> Why do we have to run static checks with Breeze when it's possible to run via pre-commit. Only a few checks like flake8, mypy requires to build an image while others can be run. Is there any specific reason to support static-checks via Breeze commands?

Actually the only reason is auto-complete. Even the few "image" pre-commits can be run with pre-commit (they will just fail in case image is not built). But I found this really problematic that someone who wants to run specific pre-commit has to look it up elsewhere (pre-commit has no support for auto-complete and apparently author of pre-commit is rather "cold" when it comes to supporting it.

The pre-commit's autocomplete issue is here https://github.com/pre-commit/pre-commit/issues/1119 (you can see my comments there) - 2.5 years and the response from @asotile was rather "cold" towards it (and he refused yaml parsing and click idea). While I love pre-commit in general, this is the only feature i actually miss. In our case the lack of autocomplete with our 100 (just counted !!!!) pre-commits where we continue to add them with a few pre-commits a month, the only way we can do it is by parsing the yaml. And since we are already using click and autocomplete in the new Breeze, I think this is the easiest way.

> @potiuk Do you think we have to parse the .pre-commit.yml file on the fly and enable auto-complete for those checks rather than adding click options ?

We can do something else. As @asotile mentioned in the comment - parsing the yaml on the flight might be slow. So one other option could be that we are using (yes!) pre-commit to generate list of pre-commits. We already do that actually in https://github.com/apache/airflow/blob/main/scripts/ci/pre_commit/pre_commit_check_pre_commit_hook_names.py where we check if all names of pre-commit are generated in `./breeze-copmplete` and documented in the docs. We could actually modify that one to generate a python file with list of pre-commits that we could import in Breeze (not only check it):

Something like that:
```
PRE_COMMIT_LIST=[
'identity',
'check-hooks-apply', .
....
]
```

then we could import it from that file and add as valid click options. That woudl likely be best way.

Additionally (not a must but should be easy), the whole index of pre-commits in our docs could be auto-generted. We could hard-code the names of tess that need breeze image and read the names and description from the yaml file: https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#available-pre-commit-checks

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Bowrna commented on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

Bowrna commented on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1012204481


   @potiuk could i use jinja2 template to generate the python code?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Bowrna commented on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

Bowrna commented on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1008273999


   I will pick up this task @potiuk 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1010978696

> @potiuk Do you think we have to parse the .pre-commit.yml file on the fly and enable auto-complete for those checks rather than adding click options ?

Something like that:
```
PRE_COMMIT_LIST=[
'identity',
'check-hooks-apply', .
....
]
```

then we could import it from that file and add as valid click options. That woudl likely be best way.

Additionally (not a must but should be easy), the whole index of pre-commits in our docs could be auto-generted. Currently we just check if the pre-commits are there, but instead (as we do with a few other auto-generated documentation) we could hard-code the names of tess that need breeze image and read the names and description from the yaml file and generate the table automatically: https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#available-pre-commit-checks

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1010978696

> @potiuk Do you think we have to parse the .pre-commit.yml file on the fly and enable auto-complete for those checks rather than adding click options ?

We can do something else. As @asotile mentioned in the comment - parsing the yaml on the flight might be slow. So one other option could be that we are using (yes!) pre-commit to generate list of pre-commits. We already do that actually in https://github.com/apache/airflow/blob/main/scripts/ci/pre_commit/pre_commit_check_pre_commit_hook_names.py where we check if all namse of pre-commit are generated in `./breeze-copmplete` and documented in the docs. We could actually modify that one to generate a python file with list of pre-commits that we could import in Breeze (not only check it):

Something like that:
```
PRE_COMMIT_LIST=[
'identity',
'check-hooks-apply', .
....
]
```

then we could import it from that file and add as valid click options. That woudl likely be best way.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1012233559


   Yep. It's best practice :)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Bowrna commented on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

Bowrna commented on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1010789046

Static Checks:

docker_engine_resources::check_all_resources
breeze::make_sure_precommit_is_installed
breeze::run_static_checks

Docker_engine_resources::check_all_resources:
Run the docker command in Python to determine the resource availability.
EXTRA_DOCKER_FLAGS
AIRFLOW_CI_IMAGE_WITH_TAG

This runs airflow ci image with tag with docker extra flags and entry point as /bin/bash and executes the script in the path /opt/airflow/scripts/in_container/run_resource_check.sh

This is an in_container script and after running docker it will be taken care of. I have to work on starting the docker run command alone. These scripts will be taken care of inside the docker container.

What's in the script?
Resources checked: Memory, CPU, Diskspace

Min Threshold of resources to run:
Memory : 4GB
CPU: 2
Disk available: 20 GB

Breeze::make_sure_precommit_is_installed
Check if pip or pip3 is in the path. Error out if not available. Then pip install –upgrade pre-commit to install pre-commit.

path is updated with ~/.local/bin to the path in case pip is run outside of virtualenv. (Do we have to do this step when converting to Python?, Also not clear how this works, could you explain it to me?)

Breeze::run_static_checks
EXTRA_STATIC_CHECK_OPTIONS - Not sure where this is initialized. Could you point me to where it’s initialized in code?
For all case, Pre commit run with EXTRA_STATIC_CHECK_OPTIONS

If static_check is mypy or flake8, then run Pre commit build first and then run Pre commit run with static_check and EXTRA_STATIC_CHECK_OPTIONS

@potiuk I have done the first check in current breeze and picked the current flow. Could you check and tell if my understanding is correct? Also can you tell if have to follow the similar flow in Python too?

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk closed issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

potiuk closed issue #20740:
URL: https://github.com/apache/airflow/issues/20740


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1010978696

> @potiuk Do you think we have to parse the .pre-commit.yml file on the fly and enable auto-complete for those checks rather than adding click options ?

We can do both actually. As @asotile mentioned in the comment - parsing the yaml on the flight might be slow. So one other option could be that we are using (yes!) pre-commit to generate list of pre-commits. We already do that actually in https://github.com/apache/airflow/blob/main/scripts/ci/pre_commit/pre_commit_check_pre_commit_hook_names.py where we check if all namse of pre-commit are generated in `./breeze-copmplete` and documented in the docs. We could actually modify that one to generate a python file with list of pre-commits that we could import in Breeze (not only check it):

Something like that:
```
PRE_COMMIT_LIST=[
'identity',
'check-hooks-apply', .
....
]
```

then we could import it from that file and add as valid click options. That woudl likely be best way.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Bowrna commented on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

Bowrna commented on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1010955332


   @potiuk I have one more question.
   
   Why do we have to run static checks with Breeze when it's possible to run via pre-commit. Only a few checks like flake8, mypy requires to build an image while others can be run. Is there any specific reason to support static-checks via Breeze commands?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1010978696

@potiuk Do you think we have to parse the .pre-commit.yml file on the fly and enable auto-complete for those checks rather than adding click options ?

Something like that:
```
PRE_COMMIT_LIST=[
'identity',
'check-hooks-apply', .
....
]
```

then we could import it from that file and add as valid click options. That woudl likely be best way.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1010978696

Actually the only reason is actually auto-complete. Even the few "image" pre-commits can be run with pre-commit (they will just fail in case image is not built). But I found this really problematic that someone who wants to run specific pre-commit has to look it up elsewhere (pre-commit has no support for auto-complete and apparently author of pre-commit is rather "cold" when it comes to supporting it.

@potiuk Do you think we have to parse the .pre-commit.yml file on the fly and enable auto-complete for those checks rather than adding click options ?

Something like that:
```
PRE_COMMIT_LIST=[
'identity',
'check-hooks-apply', .
....
]
```

then we could import it from that file and add as valid click options. That woudl likely be best way.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Bowrna commented on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

Bowrna commented on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1011925924


   Is generating a python file from code with the list of pre-commits is a safe way to do it? @potiuk 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1010978696

The pre-commit's autocomplete issue is here https://github.com/pre-commit/pre-commit/issues/1119 (you can see my comments there) - 2.5 years and the response from @asottile was rather "cold" towards it (and he refused yaml parsing and click idea). While I love pre-commit in general, this is the only feature i actually miss. In our case the lack of autocomplete with our 100 (just counted !!!!) pre-commits where we continue to add them with a few pre-commits a month, the only way we can do it is by parsing the yaml. And since we are already using click and autocomplete in the new Breeze, I think this is the easiest way.

> @potiuk Do you think we have to parse the .pre-commit.yml file on the fly and enable auto-complete for those checks rather than adding click options ?

We can do something else. As @asottile mentioned in the comment - parsing the yaml on the flight might be slow. So one other option could be that we are using (yes!) pre-commit to generate list of pre-commits. We already do that actually in https://github.com/apache/airflow/blob/main/scripts/ci/pre_commit/pre_commit_check_pre_commit_hook_names.py where we check if all names of pre-commit are generated in `./breeze-copmplete` and documented in the docs. We could actually modify that one to generate a python file with list of pre-commits that we could import in Breeze (not only check it):

Something like that:
```
PRE_COMMIT_LIST=[
'identity',
'check-hooks-apply', .
....
]
```

then we could import it from that file and add as valid click options. That woudl likely be best way.

UPDATE: https://github.com/apache/airflow/blob/main/scripts/ci/pre_commit/supported_versions.py - this is an example table of supported versions that we generate in pre-commit so w should do it in a very similar fashion.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1010978696

> @potiuk Do you think we have to parse the .pre-commit.yml file on the fly and enable auto-complete for those checks rather than adding click options ?

Something like that:
```
PRE_COMMIT_LIST=[
'identity',
'check-hooks-apply', .
....
]
```

then we could import it from that file and add as valid click options. That woudl likely be best way.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #20740: Breeze: Running static checks with Breeze

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #20740:
URL: https://github.com/apache/airflow/issues/20740#issuecomment-1011935736


   > Is generating a python file from code with the list of pre-commits is a safe way to do it? @potiuk
   
   Perfectly safe.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org