Fix all Amazon Provider MyPy errors by potiuk · Pull Request #20935 · apache/airflow

potiuk · 2022-01-18T19:47:52Z

Part of #19891

^ 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 · 2022-01-19T08:43:16Z

All green now!

potiuk · 2022-01-19T11:58:41Z

With this one (and some merged yesterday) will get to 29 MyPy errors left !

Part of apache#19891

potiuk · 2022-01-20T19:22:01Z

Looks like going to be Green :)

potiuk · 2022-01-20T22:58:18Z

Hey @ashb - all should be good here. (and really close to finish MyPy).

github-actions · 2022-01-21T13:55:35Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

JorgenG · 2022-04-29T06:09:08Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.

Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. ~~Can you please revert this change for aws glue?~~

EDIT: What are sensible next steps? (Improving my language)

potiuk · 2022-04-29T11:07:13Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?

Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

JorgenG · 2022-04-29T11:09:41Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? 🤔

potiuk · 2022-04-29T11:09:43Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?
Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

Alternatively you can open an issue, and we can mark it as good first issue if it will be justified, and hopefully it will get fixed - but yo need to describe in detail the scenario - i think the one liner description above is not enough of the doscription what exactly got broken.

JorgenG · 2022-04-29T11:10:26Z

@potiuk Totally makes sense. I'll give it a go myself and if not I'll summarize as a better described issue. Thanks a lot for responding quickly! 👍

potiuk · 2022-04-29T11:11:01Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

No idea- maybe some of the typing in Amazon was wrong, I do not know which exact part of it you refer to so it's hard to say.

potiuk · 2022-04-29T11:14:48Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

Or maybe the way it worked before was accidental and not really supported/expected - in which case it is a no issue and it was simply wrongly used - maybe there is another "proper" way of doing it - this is what I am referring to as "you are the one that can fix it". While I think MyPy changes were neutral, it could be it is really a problem with underlying library behaviour when None is passed. That's why it needs a specific explanation and reproducible case in issue.

potiuk · 2022-04-29T11:18:51Z

Ah - I saw your detailed comment now - sure I will make a fix in a moment :)

potiuk · 2022-04-29T11:19:42Z

I did not realize you commented on the exact line - I only saw this comment and did not know what it referred to 🤣

JorgenG · 2022-04-29T11:23:31Z

Wow, thanks. That is very kind! Let me know if you want any more elaboration or investigation here. As you mention, I am not sure what is "correct" usage of this. However as an example from the boto3 start_job_run, https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/glue.html#Glue.Client.start_job_run,

There does not seem to be any requirement to provide this. I think script location is passed as an argument.

EDIT: Also as can be seen here; https://docs.aws.amazon.com/glue/latest/dg/aws-glue-programming-etl-glue-arguments.html

If providing script location, it overrides what is set on the job. This wording IMO indicates that it should be possible to not provide it. 👍

Fix error introduced in apache#20935 where script location in Glue Job could be None.

potiuk · 2022-01-18T19:47:52Z

Part of #19891

^ 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 · 2022-01-19T08:43:16Z

All green now!

potiuk · 2022-01-19T11:58:41Z

With this one (and some merged yesterday) will get to 29 MyPy errors left !

Part of apache#19891

potiuk · 2022-01-20T19:22:01Z

Looks like going to be Green :)

potiuk · 2022-01-20T22:58:18Z

Hey @ashb - all should be good here. (and really close to finish MyPy).

github-actions · 2022-01-21T13:55:35Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

JorgenG · 2022-04-29T06:09:08Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.

Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. ~~Can you please revert this change for aws glue?~~

EDIT: What are sensible next steps? (Improving my language)

potiuk · 2022-04-29T11:07:13Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?

Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

JorgenG · 2022-04-29T11:09:41Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? 🤔

potiuk · 2022-04-29T11:09:43Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?
Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

Alternatively you can open an issue, and we can mark it as good first issue if it will be justified, and hopefully it will get fixed - but yo need to describe in detail the scenario - i think the one liner description above is not enough of the doscription what exactly got broken.

JorgenG · 2022-04-29T11:10:26Z

@potiuk Totally makes sense. I'll give it a go myself and if not I'll summarize as a better described issue. Thanks a lot for responding quickly! 👍

potiuk · 2022-04-29T11:11:01Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

No idea- maybe some of the typing in Amazon was wrong, I do not know which exact part of it you refer to so it's hard to say.

potiuk · 2022-04-29T11:14:48Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

Or maybe the way it worked before was accidental and not really supported/expected - in which case it is a no issue and it was simply wrongly used - maybe there is another "proper" way of doing it - this is what I am referring to as "you are the one that can fix it". While I think MyPy changes were neutral, it could be it is really a problem with underlying library behaviour when None is passed. That's why it needs a specific explanation and reproducible case in issue.

potiuk · 2022-04-29T11:18:51Z

Ah - I saw your detailed comment now - sure I will make a fix in a moment :)

potiuk · 2022-04-29T11:19:42Z

I did not realize you commented on the exact line - I only saw this comment and did not know what it referred to 🤣

JorgenG · 2022-04-29T11:23:31Z

Wow, thanks. That is very kind! Let me know if you want any more elaboration or investigation here. As you mention, I am not sure what is "correct" usage of this. However as an example from the boto3 start_job_run, https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/glue.html#Glue.Client.start_job_run,

There does not seem to be any requirement to provide this. I think script location is passed as an argument.

EDIT: Also as can be seen here; https://docs.aws.amazon.com/glue/latest/dg/aws-glue-programming-etl-glue-arguments.html

If providing script location, it overrides what is set on the job. This wording IMO indicates that it should be possible to not provide it. 👍

Fix error introduced in apache#20935 where script location in Glue Job could be None.

potiuk · 2022-01-18T19:47:52Z

Part of #19891

^ 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 · 2022-01-19T08:43:16Z

All green now!

potiuk · 2022-01-19T11:58:41Z

With this one (and some merged yesterday) will get to 29 MyPy errors left !

Part of apache#19891

potiuk · 2022-01-20T19:22:01Z

Looks like going to be Green :)

potiuk · 2022-01-20T22:58:18Z

Hey @ashb - all should be good here. (and really close to finish MyPy).

github-actions · 2022-01-21T13:55:35Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

JorgenG · 2022-04-29T06:09:08Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.

Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. ~~Can you please revert this change for aws glue?~~

EDIT: What are sensible next steps? (Improving my language)

potiuk · 2022-04-29T11:07:13Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?

Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

JorgenG · 2022-04-29T11:09:41Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? 🤔

potiuk · 2022-04-29T11:09:43Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?
Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

Alternatively you can open an issue, and we can mark it as good first issue if it will be justified, and hopefully it will get fixed - but yo need to describe in detail the scenario - i think the one liner description above is not enough of the doscription what exactly got broken.

JorgenG · 2022-04-29T11:10:26Z

@potiuk Totally makes sense. I'll give it a go myself and if not I'll summarize as a better described issue. Thanks a lot for responding quickly! 👍

potiuk · 2022-04-29T11:11:01Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

No idea- maybe some of the typing in Amazon was wrong, I do not know which exact part of it you refer to so it's hard to say.

potiuk · 2022-04-29T11:14:48Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

Or maybe the way it worked before was accidental and not really supported/expected - in which case it is a no issue and it was simply wrongly used - maybe there is another "proper" way of doing it - this is what I am referring to as "you are the one that can fix it". While I think MyPy changes were neutral, it could be it is really a problem with underlying library behaviour when None is passed. That's why it needs a specific explanation and reproducible case in issue.

potiuk · 2022-04-29T11:18:51Z

Ah - I saw your detailed comment now - sure I will make a fix in a moment :)

potiuk · 2022-04-29T11:19:42Z

I did not realize you commented on the exact line - I only saw this comment and did not know what it referred to 🤣

JorgenG · 2022-04-29T11:23:31Z

Wow, thanks. That is very kind! Let me know if you want any more elaboration or investigation here. As you mention, I am not sure what is "correct" usage of this. However as an example from the boto3 start_job_run, https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/glue.html#Glue.Client.start_job_run,

There does not seem to be any requirement to provide this. I think script location is passed as an argument.

EDIT: Also as can be seen here; https://docs.aws.amazon.com/glue/latest/dg/aws-glue-programming-etl-glue-arguments.html

If providing script location, it overrides what is set on the job. This wording IMO indicates that it should be possible to not provide it. 👍

Fix error introduced in apache#20935 where script location in Glue Job could be None.

potiuk · 2022-01-18T19:47:52Z

Part of #19891

^ 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 · 2022-01-19T08:43:16Z

All green now!

potiuk · 2022-01-19T11:58:41Z

With this one (and some merged yesterday) will get to 29 MyPy errors left !

Part of apache#19891

potiuk · 2022-01-20T19:22:01Z

Looks like going to be Green :)

potiuk · 2022-01-20T22:58:18Z

Hey @ashb - all should be good here. (and really close to finish MyPy).

github-actions · 2022-01-21T13:55:35Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

JorgenG · 2022-04-29T06:09:08Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.

Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. ~~Can you please revert this change for aws glue?~~

EDIT: What are sensible next steps? (Improving my language)

potiuk · 2022-04-29T11:07:13Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?

Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

JorgenG · 2022-04-29T11:09:41Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? 🤔

potiuk · 2022-04-29T11:09:43Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?
Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

Alternatively you can open an issue, and we can mark it as good first issue if it will be justified, and hopefully it will get fixed - but yo need to describe in detail the scenario - i think the one liner description above is not enough of the doscription what exactly got broken.

JorgenG · 2022-04-29T11:10:26Z

@potiuk Totally makes sense. I'll give it a go myself and if not I'll summarize as a better described issue. Thanks a lot for responding quickly! 👍

potiuk · 2022-04-29T11:11:01Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

No idea- maybe some of the typing in Amazon was wrong, I do not know which exact part of it you refer to so it's hard to say.

potiuk · 2022-04-29T11:14:48Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

Or maybe the way it worked before was accidental and not really supported/expected - in which case it is a no issue and it was simply wrongly used - maybe there is another "proper" way of doing it - this is what I am referring to as "you are the one that can fix it". While I think MyPy changes were neutral, it could be it is really a problem with underlying library behaviour when None is passed. That's why it needs a specific explanation and reproducible case in issue.

potiuk · 2022-04-29T11:18:51Z

Ah - I saw your detailed comment now - sure I will make a fix in a moment :)

potiuk · 2022-04-29T11:19:42Z

I did not realize you commented on the exact line - I only saw this comment and did not know what it referred to 🤣

JorgenG · 2022-04-29T11:23:31Z

Wow, thanks. That is very kind! Let me know if you want any more elaboration or investigation here. As you mention, I am not sure what is "correct" usage of this. However as an example from the boto3 start_job_run, https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/glue.html#Glue.Client.start_job_run,

There does not seem to be any requirement to provide this. I think script location is passed as an argument.

EDIT: Also as can be seen here; https://docs.aws.amazon.com/glue/latest/dg/aws-glue-programming-etl-glue-arguments.html

If providing script location, it overrides what is set on the job. This wording IMO indicates that it should be possible to not provide it. 👍

Fix error introduced in apache#20935 where script location in Glue Job could be None.

potiuk · 2022-01-18T19:47:52Z

Part of #19891

^ 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 · 2022-01-19T08:43:16Z

All green now!

potiuk · 2022-01-19T11:58:41Z

With this one (and some merged yesterday) will get to 29 MyPy errors left !

Part of apache#19891

potiuk · 2022-01-20T19:22:01Z

Looks like going to be Green :)

potiuk · 2022-01-20T22:58:18Z

Hey @ashb - all should be good here. (and really close to finish MyPy).

github-actions · 2022-01-21T13:55:35Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

JorgenG · 2022-04-29T06:09:08Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.

Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. ~~Can you please revert this change for aws glue?~~

EDIT: What are sensible next steps? (Improving my language)

potiuk · 2022-04-29T11:07:13Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?

Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

JorgenG · 2022-04-29T11:09:41Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? 🤔

potiuk · 2022-04-29T11:09:43Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?
Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

Alternatively you can open an issue, and we can mark it as good first issue if it will be justified, and hopefully it will get fixed - but yo need to describe in detail the scenario - i think the one liner description above is not enough of the doscription what exactly got broken.

JorgenG · 2022-04-29T11:10:26Z

@potiuk Totally makes sense. I'll give it a go myself and if not I'll summarize as a better described issue. Thanks a lot for responding quickly! 👍

potiuk · 2022-04-29T11:11:01Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

No idea- maybe some of the typing in Amazon was wrong, I do not know which exact part of it you refer to so it's hard to say.

potiuk · 2022-04-29T11:14:48Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

Or maybe the way it worked before was accidental and not really supported/expected - in which case it is a no issue and it was simply wrongly used - maybe there is another "proper" way of doing it - this is what I am referring to as "you are the one that can fix it". While I think MyPy changes were neutral, it could be it is really a problem with underlying library behaviour when None is passed. That's why it needs a specific explanation and reproducible case in issue.

potiuk · 2022-04-29T11:18:51Z

Ah - I saw your detailed comment now - sure I will make a fix in a moment :)

potiuk · 2022-04-29T11:19:42Z

I did not realize you commented on the exact line - I only saw this comment and did not know what it referred to 🤣

JorgenG · 2022-04-29T11:23:31Z

Wow, thanks. That is very kind! Let me know if you want any more elaboration or investigation here. As you mention, I am not sure what is "correct" usage of this. However as an example from the boto3 start_job_run, https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/glue.html#Glue.Client.start_job_run,

There does not seem to be any requirement to provide this. I think script location is passed as an argument.

EDIT: Also as can be seen here; https://docs.aws.amazon.com/glue/latest/dg/aws-glue-programming-etl-glue-arguments.html

If providing script location, it overrides what is set on the job. This wording IMO indicates that it should be possible to not provide it. 👍

Fix error introduced in apache#20935 where script location in Glue Job could be None.

potiuk · 2022-01-18T19:47:52Z

Part of #19891

^ 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 · 2022-01-19T08:43:16Z

All green now!

potiuk · 2022-01-19T11:58:41Z

With this one (and some merged yesterday) will get to 29 MyPy errors left !

Part of apache#19891

potiuk · 2022-01-20T19:22:01Z

Looks like going to be Green :)

potiuk · 2022-01-20T22:58:18Z

Hey @ashb - all should be good here. (and really close to finish MyPy).

github-actions · 2022-01-21T13:55:35Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

JorgenG · 2022-04-29T06:09:08Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.

Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. ~~Can you please revert this change for aws glue?~~

EDIT: What are sensible next steps? (Improving my language)

potiuk · 2022-04-29T11:07:13Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?

Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

JorgenG · 2022-04-29T11:09:41Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? 🤔

potiuk · 2022-04-29T11:09:43Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?
Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

Alternatively you can open an issue, and we can mark it as good first issue if it will be justified, and hopefully it will get fixed - but yo need to describe in detail the scenario - i think the one liner description above is not enough of the doscription what exactly got broken.

JorgenG · 2022-04-29T11:10:26Z

@potiuk Totally makes sense. I'll give it a go myself and if not I'll summarize as a better described issue. Thanks a lot for responding quickly! 👍

potiuk · 2022-04-29T11:11:01Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

No idea- maybe some of the typing in Amazon was wrong, I do not know which exact part of it you refer to so it's hard to say.

potiuk · 2022-04-29T11:14:48Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

Or maybe the way it worked before was accidental and not really supported/expected - in which case it is a no issue and it was simply wrongly used - maybe there is another "proper" way of doing it - this is what I am referring to as "you are the one that can fix it". While I think MyPy changes were neutral, it could be it is really a problem with underlying library behaviour when None is passed. That's why it needs a specific explanation and reproducible case in issue.

potiuk · 2022-04-29T11:18:51Z

Ah - I saw your detailed comment now - sure I will make a fix in a moment :)

potiuk · 2022-04-29T11:19:42Z

I did not realize you commented on the exact line - I only saw this comment and did not know what it referred to 🤣

JorgenG · 2022-04-29T11:23:31Z

Wow, thanks. That is very kind! Let me know if you want any more elaboration or investigation here. As you mention, I am not sure what is "correct" usage of this. However as an example from the boto3 start_job_run, https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/glue.html#Glue.Client.start_job_run,

There does not seem to be any requirement to provide this. I think script location is passed as an argument.

EDIT: Also as can be seen here; https://docs.aws.amazon.com/glue/latest/dg/aws-glue-programming-etl-glue-arguments.html

If providing script location, it overrides what is set on the job. This wording IMO indicates that it should be possible to not provide it. 👍

Fix error introduced in apache#20935 where script location in Glue Job could be None.

potiuk · 2022-01-18T19:47:52Z

Part of #19891

^ 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 · 2022-01-19T08:43:16Z

All green now!

potiuk · 2022-01-19T11:58:41Z

With this one (and some merged yesterday) will get to 29 MyPy errors left !

Part of apache#19891

potiuk · 2022-01-20T19:22:01Z

Looks like going to be Green :)

potiuk · 2022-01-20T22:58:18Z

Hey @ashb - all should be good here. (and really close to finish MyPy).

github-actions · 2022-01-21T13:55:35Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

JorgenG · 2022-04-29T06:09:08Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.

Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. ~~Can you please revert this change for aws glue?~~

EDIT: What are sensible next steps? (Improving my language)

potiuk · 2022-04-29T11:07:13Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?

Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

JorgenG · 2022-04-29T11:09:41Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? 🤔

potiuk · 2022-04-29T11:09:43Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?
Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

Alternatively you can open an issue, and we can mark it as good first issue if it will be justified, and hopefully it will get fixed - but yo need to describe in detail the scenario - i think the one liner description above is not enough of the doscription what exactly got broken.

JorgenG · 2022-04-29T11:10:26Z

@potiuk Totally makes sense. I'll give it a go myself and if not I'll summarize as a better described issue. Thanks a lot for responding quickly! 👍

potiuk · 2022-04-29T11:11:01Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

No idea- maybe some of the typing in Amazon was wrong, I do not know which exact part of it you refer to so it's hard to say.

potiuk · 2022-04-29T11:14:48Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

Or maybe the way it worked before was accidental and not really supported/expected - in which case it is a no issue and it was simply wrongly used - maybe there is another "proper" way of doing it - this is what I am referring to as "you are the one that can fix it". While I think MyPy changes were neutral, it could be it is really a problem with underlying library behaviour when None is passed. That's why it needs a specific explanation and reproducible case in issue.

potiuk · 2022-04-29T11:18:51Z

Ah - I saw your detailed comment now - sure I will make a fix in a moment :)

potiuk · 2022-04-29T11:19:42Z

I did not realize you commented on the exact line - I only saw this comment and did not know what it referred to 🤣

JorgenG · 2022-04-29T11:23:31Z

Wow, thanks. That is very kind! Let me know if you want any more elaboration or investigation here. As you mention, I am not sure what is "correct" usage of this. However as an example from the boto3 start_job_run, https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/glue.html#Glue.Client.start_job_run,

There does not seem to be any requirement to provide this. I think script location is passed as an argument.

EDIT: Also as can be seen here; https://docs.aws.amazon.com/glue/latest/dg/aws-glue-programming-etl-glue-arguments.html

If providing script location, it overrides what is set on the job. This wording IMO indicates that it should be possible to not provide it. 👍

Fix error introduced in apache#20935 where script location in Glue Job could be None.

potiuk · 2022-01-18T19:47:52Z

Part of #19891

^ 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 · 2022-01-19T08:43:16Z

All green now!

potiuk · 2022-01-19T11:58:41Z

With this one (and some merged yesterday) will get to 29 MyPy errors left !

Part of apache#19891

potiuk · 2022-01-20T19:22:01Z

Looks like going to be Green :)

potiuk · 2022-01-20T22:58:18Z

Hey @ashb - all should be good here. (and really close to finish MyPy).

github-actions · 2022-01-21T13:55:35Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

JorgenG · 2022-04-29T06:09:08Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.

Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. ~~Can you please revert this change for aws glue?~~

EDIT: What are sensible next steps? (Improving my language)

potiuk · 2022-04-29T11:07:13Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?

Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

JorgenG · 2022-04-29T11:09:41Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? 🤔

potiuk · 2022-04-29T11:09:43Z

@potiuk This has broken the ability to run a glue job without specifying a script location on AWS Glue.
Previously it was allowed that this value was None, resulting in using the default script value configured in the glue service itself. Now, the only way to escape the local file upload is to start with s3://, which then obviously will break further down. Can you please revert this change for aws glue?
Feel free to provide a good fix for it. I think you are the one who can test it eeasily, so likely it's best if you fix it. Can you please make a fix @JorgenG ?

Alternatively you can open an issue, and we can mark it as good first issue if it will be justified, and hopefully it will get fixed - but yo need to describe in detail the scenario - i think the one liner description above is not enough of the doscription what exactly got broken.

JorgenG · 2022-04-29T11:10:26Z

@potiuk Totally makes sense. I'll give it a go myself and if not I'll summarize as a better described issue. Thanks a lot for responding quickly! 👍

potiuk · 2022-04-29T11:11:01Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

No idea- maybe some of the typing in Amazon was wrong, I do not know which exact part of it you refer to so it's hard to say.

potiuk · 2022-04-29T11:14:48Z

@potiuk I think can jump on that early next week. I am not familiar with the MyPy tooling though. I assume the origin of this change is due to the parameter not being optional? Would I then add back in a "violation" of your MyPy fixes? thinking

Or maybe the way it worked before was accidental and not really supported/expected - in which case it is a no issue and it was simply wrongly used - maybe there is another "proper" way of doing it - this is what I am referring to as "you are the one that can fix it". While I think MyPy changes were neutral, it could be it is really a problem with underlying library behaviour when None is passed. That's why it needs a specific explanation and reproducible case in issue.

potiuk · 2022-04-29T11:18:51Z

Ah - I saw your detailed comment now - sure I will make a fix in a moment :)

potiuk · 2022-04-29T11:19:42Z

I did not realize you commented on the exact line - I only saw this comment and did not know what it referred to 🤣

JorgenG · 2022-04-29T11:23:31Z

Wow, thanks. That is very kind! Let me know if you want any more elaboration or investigation here. As you mention, I am not sure what is "correct" usage of this. However as an example from the boto3 start_job_run, https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/glue.html#Glue.Client.start_job_run,

There does not seem to be any requirement to provide this. I think script location is passed as an argument.

EDIT: Also as can be seen here; https://docs.aws.amazon.com/glue/latest/dg/aws-glue-programming-etl-glue-arguments.html

If providing script location, it overrides what is set on the job. This wording IMO indicates that it should be possible to not provide it. 👍

Fix error introduced in apache#20935 where script location in Glue Job could be None.

boring-cyborgBot added area:providers provider:amazon AWS/Amazon - related issues labels Jan 18, 2022

potiuk force-pushed the fix-mypy-aws-provider branch from 2649921 to 7ce84ecCompare January 18, 2022 19:48

potiuk changed the title ~~Fix allo Amazon Provider MyPy errors~~Fix all Amazon Provider MyPy errorsJan 18, 2022

potiuk mentioned this pull request Jan 18, 2022
Re-enable MyPy #19891
Closed
10 tasks

potiuk force-pushed the fix-mypy-aws-provider branch 3 times, most recently from 3c5cadf to 5be3fe3Compare January 18, 2022 23:00

potiuk closed this Jan 18, 2022

potiuk reopened this Jan 18, 2022

potiuk requested review from ashb, dstandish, ephraimbuddy, kaxil and mik-laj January 19, 2022 08:43

potiuk force-pushed the fix-mypy-aws-provider branch from 5be3fe3 to 50ac6c5Compare January 19, 2022 11:55

ashb reviewed Jan 19, 2022
View reviewed changes

Comment threadairflow/providers/amazon/aws/operators/cloud_formation.py Outdated

ashb reviewed Jan 19, 2022
View reviewed changes

Comment threadairflow/providers/amazon/aws/operators/eks.py Outdated

ashb requested changes Jan 19, 2022
View reviewed changes

Comment threadairflow/providers/amazon/aws/operators/glue.py Outdated

potiuk force-pushed the fix-mypy-aws-provider branch 2 times, most recently from d317b11 to 1374e56Compare January 20, 2022 18:24

potiuk requested a review from ashb January 20, 2022 18:28

Fix all Amazon Provider MyPy errors
157b1f5
Part of apache#19891

potiuk force-pushed the fix-mypy-aws-provider branch from 1374e56 to 157b1f5Compare January 20, 2022 18:55

ashb approved these changes Jan 21, 2022
View reviewed changes

kaxil approved these changes Jan 21, 2022
View reviewed changes

github-actionsBot added the full tests needed We need to run full set of tests for this PR to merge label Jan 21, 2022

potiuk merged commit 27b77d3 into apache:mainJan 21, 2022

potiuk deleted the fix-mypy-aws-provider branch January 21, 2022 13:55

This was referenced Feb 5, 2022
Status of testing Providers that were prepared on February 05, 2022 #21348
Closed
Status of testing Providers that were prepared on February 09, 2022 #21443
Closed

jedcunningham added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Mar 1, 2022

potiuk restored the fix-mypy-aws-provider branch April 26, 2022 20:53

potiuk mentioned this pull request Apr 29, 2022
Allow back script_location in Glue to be None #23357
Merged

potiuk added a commit to potiuk/airflow that referenced this pull request Apr 29, 2022
Allow back script_location in Glue to be None
82ad5d5
Fix error introduced in apache#20935 where script location in Glue Job could be None.

potiuk deleted the fix-mypy-aws-provider branch July 29, 2022 20:02