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 2020/05/30 07:24:21 UTC
[GitHub] [airflow] BasPH opened a new pull request #9068: Make airflow/macros/hive.py Pylint compatible
BasPH opened a new pull request #9068:
URL: https://github.com/apache/airflow/pull/9068
This PR makes `airflow/macros/hive.py` Pylint compatible, and removes it from the `pylint_todo.txt` list.
Sidenote: there's several no-nos in `airflow/macros/hive.py` (e.g. assigning lambda expressions to a name), which are not picked up by Pylint. This PR _only_ fixes the Pylint issues.
---
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Target Github ISSUE in description if exists
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #9068: Make airflow/macros/hive.py Pylint compatible
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #9068:
URL: https://github.com/apache/airflow/pull/9068#issuecomment-636344030
I agree too, but I wanted to share my concerns about deleting the code. Even if a lot of people don't use it directly, the indirect profit is big in this case. We should move it to the provider, but this may be little problematic. We can use plugins to register global macros. https://airflow.readthedocs.io/en/latest/plugins.html
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BasPH closed pull request #9068: Make airflow/macros/hive.py Pylint compatible
Posted by GitBox <gi...@apache.org>.
BasPH closed pull request #9068:
URL: https://github.com/apache/airflow/pull/9068
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BasPH commented on pull request #9068: Make airflow/macros/hive.py Pylint compatible
Posted by GitBox <gi...@apache.org>.
BasPH commented on pull request #9068:
URL: https://github.com/apache/airflow/pull/9068#issuecomment-636306346
Hi @potiuk, I took a closer look at what these 3 functions actually do. I get the feeling they were added to Airflow a long time ago without much thought about consistency and general usability for others. There are 3 functions:
- `max_partition`: not used anywhere
- `_closest_date`: "internal" function, used only in `closest_ds_partition` (same module)
- `closest_ds_partition`: not used anywhere
To me the implementation of these 3 seem oddly specific to somebodies personal use case, so after closer inspection I'd opt for simply removing them from the codebase. I'll close this PR, and make another one doing that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #9068: Make airflow/macros/hive.py Pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #9068:
URL: https://github.com/apache/airflow/pull/9068#issuecomment-636321195
@BasPH -> agree with user defined macros approach. I think it should be enough indeed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #9068: Make airflow/macros/hive.py Pylint compatible
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #9068:
URL: https://github.com/apache/airflow/pull/9068#issuecomment-636325182
@BasPH These macros are part of public API.
https://airflow.readthedocs.io/en/latest/macros-ref.html
There are no references to these methods in the code, but users can use these methods because the macros module is added to the Jinja environment.
https://github.com/apache/airflow/blob/e9ecf0ae10dffcd9ccaf22cc9f19f140fbc5ee55/airflow/models/taskinstance.py#L1394
In my opinion, they can be an example for other users if they want to see an example based on a real use case.
We had similar discussions about the plugin. which is part of Airflow. This is very specific, but it is part of Airflow to inspire other people
https://github.com/apache/airflow/tree/master/airflow/contrib/plugins/metastore_browser
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #9068: Make airflow/macros/hive.py Pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #9068:
URL: https://github.com/apache/airflow/pull/9068#issuecomment-636312271
Might be a good idea but our deprecation policy (maybe not fully documented yet) is that we should deprecate rather than remove immediately. Though I agree some of the code might not be useful at all.
@KevinYang21 I believe you were adding/modifying it in the past. Do you think this belongs to core? Or is it safe to remove it or should we deprecate or maybe move to "apache.hive" provider? Is it generic enough to be useful for others?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #9068: Make airflow/macros/hive.py Pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #9068:
URL: https://github.com/apache/airflow/pull/9068#issuecomment-636334660
IMHO if we want to make some examples, then they should be in "examples" folder if we want them to be in the core. Maybe even as symbolic links to proper "provider" package. Anyway - we should clearly say that this is an example implementation that can be used to create new plugins/macros
But I think eventually airflow core should only consist of core classes and all "provider" specific things should be moved there (if we find them generally useful).
WDYT ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BasPH commented on pull request #9068: Make airflow/macros/hive.py Pylint compatible
Posted by GitBox <gi...@apache.org>.
BasPH commented on pull request #9068:
URL: https://github.com/apache/airflow/pull/9068#issuecomment-636320359
Remove _and deprecate_, that's what I meant :-)
My 2c on the macros: I understand they can be useful, but have the same feeling about them as "helpers" and "utils" (i.e. a bunch of random functions without clear meaning), therefore would prefer to reduce the number of macros to a minimum. They can quickly become specific to somebody's own Airflow setup. I think having the `user_defined_macros` argument is enough to support users providing their own macros.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BasPH commented on pull request #9068: Make airflow/macros/hive.py Pylint compatible
Posted by GitBox <gi...@apache.org>.
BasPH commented on pull request #9068:
URL: https://github.com/apache/airflow/pull/9068#issuecomment-636342046
Agree with @potiuk - I'd even be fine with just examples of how to implement and use your own macros in the docs and removing the code from `/airflow`. Let's try to keep the "core" as clean as possible.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #9068: Make airflow/macros/hive.py Pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #9068:
URL: https://github.com/apache/airflow/pull/9068#issuecomment-636312606
Also another topic to discuss I am not sure - is anything special with "macros" package (like everything it becomes automatically available as Jinja macro?). Is it worth to have similar mechanism in providers (macros per provider ?) . I think that's an interesting question to discuss :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org