Skip to content

Conversation

@josh-fell
Copy link
Contributor

Related: #19891

In a previous PR to fix Mypy errors, the region arg was removed from default_args in the Alibaba example DAGs. While this did alleviate Mypy errors it removed a valid DAG-authoring feature that was being showcased. This PR returns the region arg to default_args as well as handling Mypy errors.

Note: Generally it seems like the first approach to handle these call-arg errors from Mypy related to default_args functionality is to add the ignore comments where appropriate. A proper fix for these scenarios seems very non-trivial and this is a good interim solution.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@potiuk
Copy link
Member

potiuk commented Dec 20, 2021

Should we make region Optional and raise Runtime Error instead as I did in #20422 ?

@potiuk
Copy link
Member

potiuk commented Dec 20, 2021

@potiuk
Copy link
Member

potiuk commented Dec 20, 2021

Ah nice. Using stub files seems to be a better solution. I will use that in #20422 too

@josh-fell
Copy link
Contributor Author

josh-fell commented Dec 20, 2021

Hmm. It seems that the Mypy checks in CI and running mypy --namespace-packages airflow/providers/alibaba/cloud locally in a Breeze container have different results. Still showing a call-arg error for region even with the stubs.

@josh-fell josh-fell marked this pull request as draft December 20, 2021 17:33
@potiuk
Copy link
Member

potiuk commented Dec 20, 2021

Hmm. It seems that the Mypy checks in CI and running mypy locally in Breeze have different results. Still showing a call-arg error for region even with the stubs.

Not for me #20422 It worked just fine in Breeze - same way as in CI.

It could be about .pyc files probably ? (wild guess) Did you actually use breeze or mypy in local virtualenv? And maybe you had some locally generated .pyc files ? Because in Breeze. PYTHONDONTWRITEBYTECODE=true is set, so .pyc files are not generated, but if you run it locally they coudl be

Copy link
Member

Choose a reason for hiding this comment

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

Ah noe

Suggested change
region: str = ...,
region: Optional[str] = None

Copy link
Member

Choose a reason for hiding this comment

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

And others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh OK. Let me try this!

@potiuk
Copy link
Member

potiuk commented Dec 20, 2021

Hmm. It seems that the Mypy checks in CI and running mypy --namespace-packages airflow/providers/alibaba/cloud locally in a Breeze container have different results. Still showing a call-arg error for region even with the stubs.

One more comment - what worked for me, I set the fields to be "Optional[str] = None" in stubs. That was what worked perfectly for me.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Dec 20, 2021
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@josh-fell josh-fell marked this pull request as ready for review December 20, 2021 20:05
@josh-fell josh-fell marked this pull request as draft December 20, 2021 20:21
@josh-fell josh-fell force-pushed the alibaba-cloud-example-dag-default-args branch from 2dcbb08 to cf8da8c Compare December 27, 2021 00:37
@josh-fell
Copy link
Contributor Author

There was a typo in an import within the stub file which was causing the stub to not be recognized. @potiuk can you review again when you get a chance please?

@josh-fell josh-fell marked this pull request as ready for review December 27, 2021 00:40
@josh-fell josh-fell requested a review from potiuk December 27, 2021 00:40
@potiuk potiuk merged commit b5d520c into apache:main Dec 28, 2021
@josh-fell josh-fell deleted the alibaba-cloud-example-dag-default-args branch December 29, 2021 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers okay to merge It's ok to merge this PR as it does not require more tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants