Skip to content

Remove redshift async test job - #30127

Merged
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci
Mar 16, 2023
Merged

Remove redshift async test job#30127
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci

Conversation

@pankajastro

@pankajastropankajastro commented Mar 15, 2023

Copy link
Copy Markdown
Member

Address #28850 (comment)


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Comment threadsetup.py Outdated

@eladkaleladkalMar 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest to sync with AWS before doing further work.
We are not yet sure if we should add this lib
#30032 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pankajastropankajastroMar 15, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hi @eladkal, I agree with your concern. I was testing the workaround suggested by @potiuk here #28850 (comment) to run the test

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see that this is already merged, but can we please wait until 3/22 before making more deferrable contributions? Recent changes from @potiuk in #30144 do alleviate many concerns, but there is always a chance that users won't be able to access new Boto functionality. We are in the process of sharing the raised concerns internally at AWS. However, I do understand that there is no better alternative than using aiobotocore and tradeoff is not bad after the CI job separation. cc: @syedahsn@o-nikolas

@potiukpotiukMar 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One more thing here. Technically with the job implemented as it is right now wiht my #30144 we have another possibility. We could reverse the approach.

Currently we remove aiobotocore in the separate job and upgrade to latest boto and run all the relevant provider tests.

But we could do it the opposite: we could use latest boto/botocore in "main", remove aibotocore from the "devel" set of dependencies and install aiobotocore (together with downgrading boto/botocore) in the separate job.

This has slightly different characteristics:

  • by default the constraints we publish will not have aiobotocore and willhave botocore/boto not compatible with the latest aiobotocore - this means that if someone would like to use deferrable operators from AWS would have to deliberately install aiobotocore and downgrade boto/botocore with it.
  • the separate job would test automatically if the "aiobotocore" compatible boto still works with all the unit-tested provider operators - this way any PR that would be based on newer features would fail at PR time.
  • then we could make a deliberate decision that this is ok (and add conditional skips in the unit tests) if that happens

So basically this is this trade-off:

  1. (current) deferrable operators work out-of-the-box with the "official constraints" (but without latest boto/botocore)
  2. (possible) - deferrable operators will not work with official constraints and they require deliberately installing aiobotocore (and downgrading boto/botocore) - but latest botocore is used in those constraints

Those are the two trade-offs, and we can still change the decision (easily) which way go. With #30144 this is as easy as changing few lines in the CI scripts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think shipping aiobotocore by default is the better option. It will be a smoother user experience if users don't have to change their workflows to use deferrables if their workflows use features not covered by aiobotocore. It's not an ideal situation in either case, but this option is less likely to cause issues for users wanting to use deferrable operators.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am with @o-nikolas on this one -- a case where everything works out of the box is better than if a user has to make choices depending.

Also, we want to increase and drive the adoption of Deferrable operators, and decreasing road-blocks for user's adopting it would be my preference

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agree with the thoughts expressed by @o-nikolas@syedahsn and @kaxil. Priority should be given to providing a better out-of-the-box user experience. If we don't make aiobotocore the default option, it will increase the workload for users, and as Niko mentioned, it may require refactoring of DAGs once the user enables async. On the other hand, we don't have any evidence yet that restricting the botocore version will cause significant customer problems.

Thank you @potiuk for spending time on this and providing us with options. We're certainly converging to a better approach than what we started with.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah. I did not want to bias it, but that was also my personal preference (but I wanted to objectively show you the options). It seems much easier to explain as well, and I have a feeling that deferring new features by few months is not a huge issue as the adoption is usually anyhow delayed and deferrable operators not being available by default would be a big bummer.

With the #30161 now merged, the aiobotocore will also be included in the PROD image by default, so we are on track to increase the adoption of deferrable operators :).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is awesome, look like we have solution now. Thank you so much @potiuk for stepping-up bringing it in better shape it was long since pending 🚀

@pankajastropankajastro changed the title [WIP] Fix redshiftRemove redshift async test jobMar 16, 2023

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I treat @o-nikolas approval on #29923 as general OK from the AWS team to follow this approach.

@potiuk
potiuk merged commit b54285d into apache:mainMar 16, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Mar 17, 2023
This is follow-up after apache#30127 and apache#30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
apache#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
potiuk added a commit that referenced this pull request Mar 17, 2023
This is follow-up after #30127 and #30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
@pankajastro
pankajastro deleted the fix_async_redshift_ci branch March 17, 2023 19:03
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@pankajastro@potiuk@kaxil@eladkal@o-nikolas@shubham22@syedahsn@ephraimbuddy
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Remove redshift async test job by pankajastro · Pull Request #30127 · apache/airflow · GitHub
Skip to content

Remove redshift async test job - #30127

Merged
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci
Mar 16, 2023
Merged

Remove redshift async test job#30127
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci

Conversation

@pankajastro

@pankajastropankajastro commented Mar 15, 2023

Copy link
Copy Markdown
Member

Address #28850 (comment)


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Comment threadsetup.py Outdated

@eladkaleladkalMar 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest to sync with AWS before doing further work.
We are not yet sure if we should add this lib
#30032 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pankajastropankajastroMar 15, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hi @eladkal, I agree with your concern. I was testing the workaround suggested by @potiuk here #28850 (comment) to run the test

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see that this is already merged, but can we please wait until 3/22 before making more deferrable contributions? Recent changes from @potiuk in #30144 do alleviate many concerns, but there is always a chance that users won't be able to access new Boto functionality. We are in the process of sharing the raised concerns internally at AWS. However, I do understand that there is no better alternative than using aiobotocore and tradeoff is not bad after the CI job separation. cc: @syedahsn@o-nikolas

@potiukpotiukMar 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One more thing here. Technically with the job implemented as it is right now wiht my #30144 we have another possibility. We could reverse the approach.

Currently we remove aiobotocore in the separate job and upgrade to latest boto and run all the relevant provider tests.

But we could do it the opposite: we could use latest boto/botocore in "main", remove aibotocore from the "devel" set of dependencies and install aiobotocore (together with downgrading boto/botocore) in the separate job.

This has slightly different characteristics:

  • by default the constraints we publish will not have aiobotocore and willhave botocore/boto not compatible with the latest aiobotocore - this means that if someone would like to use deferrable operators from AWS would have to deliberately install aiobotocore and downgrade boto/botocore with it.
  • the separate job would test automatically if the "aiobotocore" compatible boto still works with all the unit-tested provider operators - this way any PR that would be based on newer features would fail at PR time.
  • then we could make a deliberate decision that this is ok (and add conditional skips in the unit tests) if that happens

So basically this is this trade-off:

  1. (current) deferrable operators work out-of-the-box with the "official constraints" (but without latest boto/botocore)
  2. (possible) - deferrable operators will not work with official constraints and they require deliberately installing aiobotocore (and downgrading boto/botocore) - but latest botocore is used in those constraints

Those are the two trade-offs, and we can still change the decision (easily) which way go. With #30144 this is as easy as changing few lines in the CI scripts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think shipping aiobotocore by default is the better option. It will be a smoother user experience if users don't have to change their workflows to use deferrables if their workflows use features not covered by aiobotocore. It's not an ideal situation in either case, but this option is less likely to cause issues for users wanting to use deferrable operators.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am with @o-nikolas on this one -- a case where everything works out of the box is better than if a user has to make choices depending.

Also, we want to increase and drive the adoption of Deferrable operators, and decreasing road-blocks for user's adopting it would be my preference

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agree with the thoughts expressed by @o-nikolas@syedahsn and @kaxil. Priority should be given to providing a better out-of-the-box user experience. If we don't make aiobotocore the default option, it will increase the workload for users, and as Niko mentioned, it may require refactoring of DAGs once the user enables async. On the other hand, we don't have any evidence yet that restricting the botocore version will cause significant customer problems.

Thank you @potiuk for spending time on this and providing us with options. We're certainly converging to a better approach than what we started with.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah. I did not want to bias it, but that was also my personal preference (but I wanted to objectively show you the options). It seems much easier to explain as well, and I have a feeling that deferring new features by few months is not a huge issue as the adoption is usually anyhow delayed and deferrable operators not being available by default would be a big bummer.

With the #30161 now merged, the aiobotocore will also be included in the PROD image by default, so we are on track to increase the adoption of deferrable operators :).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is awesome, look like we have solution now. Thank you so much @potiuk for stepping-up bringing it in better shape it was long since pending 🚀

@pankajastropankajastro changed the title [WIP] Fix redshiftRemove redshift async test jobMar 16, 2023

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I treat @o-nikolas approval on #29923 as general OK from the AWS team to follow this approach.

@potiuk
potiuk merged commit b54285d into apache:mainMar 16, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Mar 17, 2023
This is follow-up after apache#30127 and apache#30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
apache#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
potiuk added a commit that referenced this pull request Mar 17, 2023
This is follow-up after #30127 and #30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
@pankajastro
pankajastro deleted the fix_async_redshift_ci branch March 17, 2023 19:03
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@pankajastro@potiuk@kaxil@eladkal@o-nikolas@shubham22@syedahsn@ephraimbuddy
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Remove redshift async test job by pankajastro · Pull Request #30127 · apache/airflow · GitHub
Skip to content

Remove redshift async test job - #30127

Merged
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci
Mar 16, 2023
Merged

Remove redshift async test job#30127
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci

Conversation

@pankajastro

@pankajastropankajastro commented Mar 15, 2023

Copy link
Copy Markdown
Member

Address #28850 (comment)


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Comment threadsetup.py Outdated

@eladkaleladkalMar 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest to sync with AWS before doing further work.
We are not yet sure if we should add this lib
#30032 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pankajastropankajastroMar 15, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hi @eladkal, I agree with your concern. I was testing the workaround suggested by @potiuk here #28850 (comment) to run the test

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see that this is already merged, but can we please wait until 3/22 before making more deferrable contributions? Recent changes from @potiuk in #30144 do alleviate many concerns, but there is always a chance that users won't be able to access new Boto functionality. We are in the process of sharing the raised concerns internally at AWS. However, I do understand that there is no better alternative than using aiobotocore and tradeoff is not bad after the CI job separation. cc: @syedahsn@o-nikolas

@potiukpotiukMar 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One more thing here. Technically with the job implemented as it is right now wiht my #30144 we have another possibility. We could reverse the approach.

Currently we remove aiobotocore in the separate job and upgrade to latest boto and run all the relevant provider tests.

But we could do it the opposite: we could use latest boto/botocore in "main", remove aibotocore from the "devel" set of dependencies and install aiobotocore (together with downgrading boto/botocore) in the separate job.

This has slightly different characteristics:

  • by default the constraints we publish will not have aiobotocore and willhave botocore/boto not compatible with the latest aiobotocore - this means that if someone would like to use deferrable operators from AWS would have to deliberately install aiobotocore and downgrade boto/botocore with it.
  • the separate job would test automatically if the "aiobotocore" compatible boto still works with all the unit-tested provider operators - this way any PR that would be based on newer features would fail at PR time.
  • then we could make a deliberate decision that this is ok (and add conditional skips in the unit tests) if that happens

So basically this is this trade-off:

  1. (current) deferrable operators work out-of-the-box with the "official constraints" (but without latest boto/botocore)
  2. (possible) - deferrable operators will not work with official constraints and they require deliberately installing aiobotocore (and downgrading boto/botocore) - but latest botocore is used in those constraints

Those are the two trade-offs, and we can still change the decision (easily) which way go. With #30144 this is as easy as changing few lines in the CI scripts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think shipping aiobotocore by default is the better option. It will be a smoother user experience if users don't have to change their workflows to use deferrables if their workflows use features not covered by aiobotocore. It's not an ideal situation in either case, but this option is less likely to cause issues for users wanting to use deferrable operators.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am with @o-nikolas on this one -- a case where everything works out of the box is better than if a user has to make choices depending.

Also, we want to increase and drive the adoption of Deferrable operators, and decreasing road-blocks for user's adopting it would be my preference

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agree with the thoughts expressed by @o-nikolas@syedahsn and @kaxil. Priority should be given to providing a better out-of-the-box user experience. If we don't make aiobotocore the default option, it will increase the workload for users, and as Niko mentioned, it may require refactoring of DAGs once the user enables async. On the other hand, we don't have any evidence yet that restricting the botocore version will cause significant customer problems.

Thank you @potiuk for spending time on this and providing us with options. We're certainly converging to a better approach than what we started with.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah. I did not want to bias it, but that was also my personal preference (but I wanted to objectively show you the options). It seems much easier to explain as well, and I have a feeling that deferring new features by few months is not a huge issue as the adoption is usually anyhow delayed and deferrable operators not being available by default would be a big bummer.

With the #30161 now merged, the aiobotocore will also be included in the PROD image by default, so we are on track to increase the adoption of deferrable operators :).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is awesome, look like we have solution now. Thank you so much @potiuk for stepping-up bringing it in better shape it was long since pending 🚀

@pankajastropankajastro changed the title [WIP] Fix redshiftRemove redshift async test jobMar 16, 2023

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I treat @o-nikolas approval on #29923 as general OK from the AWS team to follow this approach.

@potiuk
potiuk merged commit b54285d into apache:mainMar 16, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Mar 17, 2023
This is follow-up after apache#30127 and apache#30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
apache#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
potiuk added a commit that referenced this pull request Mar 17, 2023
This is follow-up after #30127 and #30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
@pankajastro
pankajastro deleted the fix_async_redshift_ci branch March 17, 2023 19:03
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@pankajastro@potiuk@kaxil@eladkal@o-nikolas@shubham22@syedahsn@ephraimbuddy
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Remove redshift async test job by pankajastro · Pull Request #30127 · apache/airflow · GitHub
Skip to content

Remove redshift async test job - #30127

Merged
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci
Mar 16, 2023
Merged

Remove redshift async test job#30127
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci

Conversation

@pankajastro

@pankajastropankajastro commented Mar 15, 2023

Copy link
Copy Markdown
Member

Address #28850 (comment)


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Comment threadsetup.py Outdated

@eladkaleladkalMar 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest to sync with AWS before doing further work.
We are not yet sure if we should add this lib
#30032 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pankajastropankajastroMar 15, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hi @eladkal, I agree with your concern. I was testing the workaround suggested by @potiuk here #28850 (comment) to run the test

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see that this is already merged, but can we please wait until 3/22 before making more deferrable contributions? Recent changes from @potiuk in #30144 do alleviate many concerns, but there is always a chance that users won't be able to access new Boto functionality. We are in the process of sharing the raised concerns internally at AWS. However, I do understand that there is no better alternative than using aiobotocore and tradeoff is not bad after the CI job separation. cc: @syedahsn@o-nikolas

@potiukpotiukMar 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One more thing here. Technically with the job implemented as it is right now wiht my #30144 we have another possibility. We could reverse the approach.

Currently we remove aiobotocore in the separate job and upgrade to latest boto and run all the relevant provider tests.

But we could do it the opposite: we could use latest boto/botocore in "main", remove aibotocore from the "devel" set of dependencies and install aiobotocore (together with downgrading boto/botocore) in the separate job.

This has slightly different characteristics:

  • by default the constraints we publish will not have aiobotocore and willhave botocore/boto not compatible with the latest aiobotocore - this means that if someone would like to use deferrable operators from AWS would have to deliberately install aiobotocore and downgrade boto/botocore with it.
  • the separate job would test automatically if the "aiobotocore" compatible boto still works with all the unit-tested provider operators - this way any PR that would be based on newer features would fail at PR time.
  • then we could make a deliberate decision that this is ok (and add conditional skips in the unit tests) if that happens

So basically this is this trade-off:

  1. (current) deferrable operators work out-of-the-box with the "official constraints" (but without latest boto/botocore)
  2. (possible) - deferrable operators will not work with official constraints and they require deliberately installing aiobotocore (and downgrading boto/botocore) - but latest botocore is used in those constraints

Those are the two trade-offs, and we can still change the decision (easily) which way go. With #30144 this is as easy as changing few lines in the CI scripts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think shipping aiobotocore by default is the better option. It will be a smoother user experience if users don't have to change their workflows to use deferrables if their workflows use features not covered by aiobotocore. It's not an ideal situation in either case, but this option is less likely to cause issues for users wanting to use deferrable operators.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am with @o-nikolas on this one -- a case where everything works out of the box is better than if a user has to make choices depending.

Also, we want to increase and drive the adoption of Deferrable operators, and decreasing road-blocks for user's adopting it would be my preference

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agree with the thoughts expressed by @o-nikolas@syedahsn and @kaxil. Priority should be given to providing a better out-of-the-box user experience. If we don't make aiobotocore the default option, it will increase the workload for users, and as Niko mentioned, it may require refactoring of DAGs once the user enables async. On the other hand, we don't have any evidence yet that restricting the botocore version will cause significant customer problems.

Thank you @potiuk for spending time on this and providing us with options. We're certainly converging to a better approach than what we started with.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah. I did not want to bias it, but that was also my personal preference (but I wanted to objectively show you the options). It seems much easier to explain as well, and I have a feeling that deferring new features by few months is not a huge issue as the adoption is usually anyhow delayed and deferrable operators not being available by default would be a big bummer.

With the #30161 now merged, the aiobotocore will also be included in the PROD image by default, so we are on track to increase the adoption of deferrable operators :).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is awesome, look like we have solution now. Thank you so much @potiuk for stepping-up bringing it in better shape it was long since pending 🚀

@pankajastropankajastro changed the title [WIP] Fix redshiftRemove redshift async test jobMar 16, 2023

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I treat @o-nikolas approval on #29923 as general OK from the AWS team to follow this approach.

@potiuk
potiuk merged commit b54285d into apache:mainMar 16, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Mar 17, 2023
This is follow-up after apache#30127 and apache#30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
apache#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
potiuk added a commit that referenced this pull request Mar 17, 2023
This is follow-up after #30127 and #30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
@pankajastro
pankajastro deleted the fix_async_redshift_ci branch March 17, 2023 19:03
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@pankajastro@potiuk@kaxil@eladkal@o-nikolas@shubham22@syedahsn@ephraimbuddy
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Remove redshift async test job by pankajastro · Pull Request #30127 · apache/airflow · GitHub
Skip to content

Remove redshift async test job - #30127

Merged
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci
Mar 16, 2023
Merged

Remove redshift async test job#30127
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci

Conversation

@pankajastro

@pankajastropankajastro commented Mar 15, 2023

Copy link
Copy Markdown
Member

Address #28850 (comment)


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Comment threadsetup.py Outdated

@eladkaleladkalMar 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest to sync with AWS before doing further work.
We are not yet sure if we should add this lib
#30032 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pankajastropankajastroMar 15, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hi @eladkal, I agree with your concern. I was testing the workaround suggested by @potiuk here #28850 (comment) to run the test

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see that this is already merged, but can we please wait until 3/22 before making more deferrable contributions? Recent changes from @potiuk in #30144 do alleviate many concerns, but there is always a chance that users won't be able to access new Boto functionality. We are in the process of sharing the raised concerns internally at AWS. However, I do understand that there is no better alternative than using aiobotocore and tradeoff is not bad after the CI job separation. cc: @syedahsn@o-nikolas

@potiukpotiukMar 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One more thing here. Technically with the job implemented as it is right now wiht my #30144 we have another possibility. We could reverse the approach.

Currently we remove aiobotocore in the separate job and upgrade to latest boto and run all the relevant provider tests.

But we could do it the opposite: we could use latest boto/botocore in "main", remove aibotocore from the "devel" set of dependencies and install aiobotocore (together with downgrading boto/botocore) in the separate job.

This has slightly different characteristics:

  • by default the constraints we publish will not have aiobotocore and willhave botocore/boto not compatible with the latest aiobotocore - this means that if someone would like to use deferrable operators from AWS would have to deliberately install aiobotocore and downgrade boto/botocore with it.
  • the separate job would test automatically if the "aiobotocore" compatible boto still works with all the unit-tested provider operators - this way any PR that would be based on newer features would fail at PR time.
  • then we could make a deliberate decision that this is ok (and add conditional skips in the unit tests) if that happens

So basically this is this trade-off:

  1. (current) deferrable operators work out-of-the-box with the "official constraints" (but without latest boto/botocore)
  2. (possible) - deferrable operators will not work with official constraints and they require deliberately installing aiobotocore (and downgrading boto/botocore) - but latest botocore is used in those constraints

Those are the two trade-offs, and we can still change the decision (easily) which way go. With #30144 this is as easy as changing few lines in the CI scripts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think shipping aiobotocore by default is the better option. It will be a smoother user experience if users don't have to change their workflows to use deferrables if their workflows use features not covered by aiobotocore. It's not an ideal situation in either case, but this option is less likely to cause issues for users wanting to use deferrable operators.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am with @o-nikolas on this one -- a case where everything works out of the box is better than if a user has to make choices depending.

Also, we want to increase and drive the adoption of Deferrable operators, and decreasing road-blocks for user's adopting it would be my preference

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agree with the thoughts expressed by @o-nikolas@syedahsn and @kaxil. Priority should be given to providing a better out-of-the-box user experience. If we don't make aiobotocore the default option, it will increase the workload for users, and as Niko mentioned, it may require refactoring of DAGs once the user enables async. On the other hand, we don't have any evidence yet that restricting the botocore version will cause significant customer problems.

Thank you @potiuk for spending time on this and providing us with options. We're certainly converging to a better approach than what we started with.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah. I did not want to bias it, but that was also my personal preference (but I wanted to objectively show you the options). It seems much easier to explain as well, and I have a feeling that deferring new features by few months is not a huge issue as the adoption is usually anyhow delayed and deferrable operators not being available by default would be a big bummer.

With the #30161 now merged, the aiobotocore will also be included in the PROD image by default, so we are on track to increase the adoption of deferrable operators :).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is awesome, look like we have solution now. Thank you so much @potiuk for stepping-up bringing it in better shape it was long since pending 🚀

@pankajastropankajastro changed the title [WIP] Fix redshiftRemove redshift async test jobMar 16, 2023

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I treat @o-nikolas approval on #29923 as general OK from the AWS team to follow this approach.

@potiuk
potiuk merged commit b54285d into apache:mainMar 16, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Mar 17, 2023
This is follow-up after apache#30127 and apache#30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
apache#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
potiuk added a commit that referenced this pull request Mar 17, 2023
This is follow-up after #30127 and #30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
@pankajastro
pankajastro deleted the fix_async_redshift_ci branch March 17, 2023 19:03
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@pankajastro@potiuk@kaxil@eladkal@o-nikolas@shubham22@syedahsn@ephraimbuddy
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Remove redshift async test job by pankajastro · Pull Request #30127 · apache/airflow · GitHub
Skip to content

Remove redshift async test job - #30127

Merged
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci
Mar 16, 2023
Merged

Remove redshift async test job#30127
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci

Conversation

@pankajastro

@pankajastropankajastro commented Mar 15, 2023

Copy link
Copy Markdown
Member

Address #28850 (comment)


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Comment threadsetup.py Outdated

@eladkaleladkalMar 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest to sync with AWS before doing further work.
We are not yet sure if we should add this lib
#30032 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pankajastropankajastroMar 15, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hi @eladkal, I agree with your concern. I was testing the workaround suggested by @potiuk here #28850 (comment) to run the test

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see that this is already merged, but can we please wait until 3/22 before making more deferrable contributions? Recent changes from @potiuk in #30144 do alleviate many concerns, but there is always a chance that users won't be able to access new Boto functionality. We are in the process of sharing the raised concerns internally at AWS. However, I do understand that there is no better alternative than using aiobotocore and tradeoff is not bad after the CI job separation. cc: @syedahsn@o-nikolas

@potiukpotiukMar 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One more thing here. Technically with the job implemented as it is right now wiht my #30144 we have another possibility. We could reverse the approach.

Currently we remove aiobotocore in the separate job and upgrade to latest boto and run all the relevant provider tests.

But we could do it the opposite: we could use latest boto/botocore in "main", remove aibotocore from the "devel" set of dependencies and install aiobotocore (together with downgrading boto/botocore) in the separate job.

This has slightly different characteristics:

  • by default the constraints we publish will not have aiobotocore and willhave botocore/boto not compatible with the latest aiobotocore - this means that if someone would like to use deferrable operators from AWS would have to deliberately install aiobotocore and downgrade boto/botocore with it.
  • the separate job would test automatically if the "aiobotocore" compatible boto still works with all the unit-tested provider operators - this way any PR that would be based on newer features would fail at PR time.
  • then we could make a deliberate decision that this is ok (and add conditional skips in the unit tests) if that happens

So basically this is this trade-off:

  1. (current) deferrable operators work out-of-the-box with the "official constraints" (but without latest boto/botocore)
  2. (possible) - deferrable operators will not work with official constraints and they require deliberately installing aiobotocore (and downgrading boto/botocore) - but latest botocore is used in those constraints

Those are the two trade-offs, and we can still change the decision (easily) which way go. With #30144 this is as easy as changing few lines in the CI scripts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think shipping aiobotocore by default is the better option. It will be a smoother user experience if users don't have to change their workflows to use deferrables if their workflows use features not covered by aiobotocore. It's not an ideal situation in either case, but this option is less likely to cause issues for users wanting to use deferrable operators.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am with @o-nikolas on this one -- a case where everything works out of the box is better than if a user has to make choices depending.

Also, we want to increase and drive the adoption of Deferrable operators, and decreasing road-blocks for user's adopting it would be my preference

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agree with the thoughts expressed by @o-nikolas@syedahsn and @kaxil. Priority should be given to providing a better out-of-the-box user experience. If we don't make aiobotocore the default option, it will increase the workload for users, and as Niko mentioned, it may require refactoring of DAGs once the user enables async. On the other hand, we don't have any evidence yet that restricting the botocore version will cause significant customer problems.

Thank you @potiuk for spending time on this and providing us with options. We're certainly converging to a better approach than what we started with.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah. I did not want to bias it, but that was also my personal preference (but I wanted to objectively show you the options). It seems much easier to explain as well, and I have a feeling that deferring new features by few months is not a huge issue as the adoption is usually anyhow delayed and deferrable operators not being available by default would be a big bummer.

With the #30161 now merged, the aiobotocore will also be included in the PROD image by default, so we are on track to increase the adoption of deferrable operators :).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is awesome, look like we have solution now. Thank you so much @potiuk for stepping-up bringing it in better shape it was long since pending 🚀

@pankajastropankajastro changed the title [WIP] Fix redshiftRemove redshift async test jobMar 16, 2023

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I treat @o-nikolas approval on #29923 as general OK from the AWS team to follow this approach.

@potiuk
potiuk merged commit b54285d into apache:mainMar 16, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Mar 17, 2023
This is follow-up after apache#30127 and apache#30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
apache#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
potiuk added a commit that referenced this pull request Mar 17, 2023
This is follow-up after #30127 and #30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
@pankajastro
pankajastro deleted the fix_async_redshift_ci branch March 17, 2023 19:03
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@pankajastro@potiuk@kaxil@eladkal@o-nikolas@shubham22@syedahsn@ephraimbuddy
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Remove redshift async test job by pankajastro · Pull Request #30127 · apache/airflow · GitHub
Skip to content

Remove redshift async test job - #30127

Merged
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci
Mar 16, 2023
Merged

Remove redshift async test job#30127
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci

Conversation

@pankajastro

@pankajastropankajastro commented Mar 15, 2023

Copy link
Copy Markdown
Member

Address #28850 (comment)


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Comment threadsetup.py Outdated

@eladkaleladkalMar 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest to sync with AWS before doing further work.
We are not yet sure if we should add this lib
#30032 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pankajastropankajastroMar 15, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hi @eladkal, I agree with your concern. I was testing the workaround suggested by @potiuk here #28850 (comment) to run the test

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see that this is already merged, but can we please wait until 3/22 before making more deferrable contributions? Recent changes from @potiuk in #30144 do alleviate many concerns, but there is always a chance that users won't be able to access new Boto functionality. We are in the process of sharing the raised concerns internally at AWS. However, I do understand that there is no better alternative than using aiobotocore and tradeoff is not bad after the CI job separation. cc: @syedahsn@o-nikolas

@potiukpotiukMar 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One more thing here. Technically with the job implemented as it is right now wiht my #30144 we have another possibility. We could reverse the approach.

Currently we remove aiobotocore in the separate job and upgrade to latest boto and run all the relevant provider tests.

But we could do it the opposite: we could use latest boto/botocore in "main", remove aibotocore from the "devel" set of dependencies and install aiobotocore (together with downgrading boto/botocore) in the separate job.

This has slightly different characteristics:

  • by default the constraints we publish will not have aiobotocore and willhave botocore/boto not compatible with the latest aiobotocore - this means that if someone would like to use deferrable operators from AWS would have to deliberately install aiobotocore and downgrade boto/botocore with it.
  • the separate job would test automatically if the "aiobotocore" compatible boto still works with all the unit-tested provider operators - this way any PR that would be based on newer features would fail at PR time.
  • then we could make a deliberate decision that this is ok (and add conditional skips in the unit tests) if that happens

So basically this is this trade-off:

  1. (current) deferrable operators work out-of-the-box with the "official constraints" (but without latest boto/botocore)
  2. (possible) - deferrable operators will not work with official constraints and they require deliberately installing aiobotocore (and downgrading boto/botocore) - but latest botocore is used in those constraints

Those are the two trade-offs, and we can still change the decision (easily) which way go. With #30144 this is as easy as changing few lines in the CI scripts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think shipping aiobotocore by default is the better option. It will be a smoother user experience if users don't have to change their workflows to use deferrables if their workflows use features not covered by aiobotocore. It's not an ideal situation in either case, but this option is less likely to cause issues for users wanting to use deferrable operators.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am with @o-nikolas on this one -- a case where everything works out of the box is better than if a user has to make choices depending.

Also, we want to increase and drive the adoption of Deferrable operators, and decreasing road-blocks for user's adopting it would be my preference

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agree with the thoughts expressed by @o-nikolas@syedahsn and @kaxil. Priority should be given to providing a better out-of-the-box user experience. If we don't make aiobotocore the default option, it will increase the workload for users, and as Niko mentioned, it may require refactoring of DAGs once the user enables async. On the other hand, we don't have any evidence yet that restricting the botocore version will cause significant customer problems.

Thank you @potiuk for spending time on this and providing us with options. We're certainly converging to a better approach than what we started with.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah. I did not want to bias it, but that was also my personal preference (but I wanted to objectively show you the options). It seems much easier to explain as well, and I have a feeling that deferring new features by few months is not a huge issue as the adoption is usually anyhow delayed and deferrable operators not being available by default would be a big bummer.

With the #30161 now merged, the aiobotocore will also be included in the PROD image by default, so we are on track to increase the adoption of deferrable operators :).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is awesome, look like we have solution now. Thank you so much @potiuk for stepping-up bringing it in better shape it was long since pending 🚀

@pankajastropankajastro changed the title [WIP] Fix redshiftRemove redshift async test jobMar 16, 2023

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I treat @o-nikolas approval on #29923 as general OK from the AWS team to follow this approach.

@potiuk
potiuk merged commit b54285d into apache:mainMar 16, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Mar 17, 2023
This is follow-up after apache#30127 and apache#30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
apache#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
potiuk added a commit that referenced this pull request Mar 17, 2023
This is follow-up after #30127 and #30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
@pankajastro
pankajastro deleted the fix_async_redshift_ci branch March 17, 2023 19:03
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@pankajastro@potiuk@kaxil@eladkal@o-nikolas@shubham22@syedahsn@ephraimbuddy
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Remove redshift async test job by pankajastro · Pull Request #30127 · apache/airflow · GitHub
Skip to content

Remove redshift async test job - #30127

Merged
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci
Mar 16, 2023
Merged

Remove redshift async test job#30127
potiuk merged 1 commit into
apache:mainfrom
astronomer:fix_async_redshift_ci

Conversation

@pankajastro

@pankajastropankajastro commented Mar 15, 2023

Copy link
Copy Markdown
Member

Address #28850 (comment)


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Comment threadsetup.py Outdated

@eladkaleladkalMar 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest to sync with AWS before doing further work.
We are not yet sure if we should add this lib
#30032 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pankajastropankajastroMar 15, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hi @eladkal, I agree with your concern. I was testing the workaround suggested by @potiuk here #28850 (comment) to run the test

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see that this is already merged, but can we please wait until 3/22 before making more deferrable contributions? Recent changes from @potiuk in #30144 do alleviate many concerns, but there is always a chance that users won't be able to access new Boto functionality. We are in the process of sharing the raised concerns internally at AWS. However, I do understand that there is no better alternative than using aiobotocore and tradeoff is not bad after the CI job separation. cc: @syedahsn@o-nikolas

@potiukpotiukMar 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One more thing here. Technically with the job implemented as it is right now wiht my #30144 we have another possibility. We could reverse the approach.

Currently we remove aiobotocore in the separate job and upgrade to latest boto and run all the relevant provider tests.

But we could do it the opposite: we could use latest boto/botocore in "main", remove aibotocore from the "devel" set of dependencies and install aiobotocore (together with downgrading boto/botocore) in the separate job.

This has slightly different characteristics:

  • by default the constraints we publish will not have aiobotocore and willhave botocore/boto not compatible with the latest aiobotocore - this means that if someone would like to use deferrable operators from AWS would have to deliberately install aiobotocore and downgrade boto/botocore with it.
  • the separate job would test automatically if the "aiobotocore" compatible boto still works with all the unit-tested provider operators - this way any PR that would be based on newer features would fail at PR time.
  • then we could make a deliberate decision that this is ok (and add conditional skips in the unit tests) if that happens

So basically this is this trade-off:

  1. (current) deferrable operators work out-of-the-box with the "official constraints" (but without latest boto/botocore)
  2. (possible) - deferrable operators will not work with official constraints and they require deliberately installing aiobotocore (and downgrading boto/botocore) - but latest botocore is used in those constraints

Those are the two trade-offs, and we can still change the decision (easily) which way go. With #30144 this is as easy as changing few lines in the CI scripts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think shipping aiobotocore by default is the better option. It will be a smoother user experience if users don't have to change their workflows to use deferrables if their workflows use features not covered by aiobotocore. It's not an ideal situation in either case, but this option is less likely to cause issues for users wanting to use deferrable operators.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am with @o-nikolas on this one -- a case where everything works out of the box is better than if a user has to make choices depending.

Also, we want to increase and drive the adoption of Deferrable operators, and decreasing road-blocks for user's adopting it would be my preference

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agree with the thoughts expressed by @o-nikolas@syedahsn and @kaxil. Priority should be given to providing a better out-of-the-box user experience. If we don't make aiobotocore the default option, it will increase the workload for users, and as Niko mentioned, it may require refactoring of DAGs once the user enables async. On the other hand, we don't have any evidence yet that restricting the botocore version will cause significant customer problems.

Thank you @potiuk for spending time on this and providing us with options. We're certainly converging to a better approach than what we started with.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah. I did not want to bias it, but that was also my personal preference (but I wanted to objectively show you the options). It seems much easier to explain as well, and I have a feeling that deferring new features by few months is not a huge issue as the adoption is usually anyhow delayed and deferrable operators not being available by default would be a big bummer.

With the #30161 now merged, the aiobotocore will also be included in the PROD image by default, so we are on track to increase the adoption of deferrable operators :).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is awesome, look like we have solution now. Thank you so much @potiuk for stepping-up bringing it in better shape it was long since pending 🚀

@pankajastropankajastro changed the title [WIP] Fix redshiftRemove redshift async test jobMar 16, 2023

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I treat @o-nikolas approval on #29923 as general OK from the AWS team to follow this approach.

@potiuk
potiuk merged commit b54285d into apache:mainMar 16, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Mar 17, 2023
This is follow-up after apache#30127 and apache#30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
apache#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
potiuk added a commit that referenced this pull request Mar 17, 2023
This is follow-up after #30127 and #30144 about handling async
(deferrable) operators for amazon provider.
Rather than making aiobotocre directly a devel extra dependency,
we create a separate "aiobotocore" extra that allows for greater
flexibility on how we handle the aiobotocore support. It allows
for two approach:
1) (current) if we decide that by default we keep boto/botocore
compatible with aiobotocore in our constraints/image then
aiobotocore should be added to devel and it should be included
as preselected extra in Dockerfile. This will lead to
having aibotocore and compatible boto/botocore in both constraints
and the PROD image.
2) (possible) if we decide that we prefer to keep to the latest
version of boto/botocore in constraints/image, then we could
remove aiobotocore from both constraints and PROD image. We should
also in this case swap the "LatestBoto" CI job introduced in
#30144 to be "WithAiobotocore" job - by installing aiobotocore
and downgrading boto/botocore in the job.
@pankajastro
pankajastro deleted the fix_async_redshift_ci branch March 17, 2023 19:03
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@pankajastro@potiuk@kaxil@eladkal@o-nikolas@shubham22@syedahsn@ephraimbuddy