Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

breeze test parallel #23715

Closed
wants to merge 2 commits into from
Closed

breeze test parallel #23715

wants to merge 2 commits into from

Conversation

Bowrna
Copy link
Contributor

@Bowrna Bowrna commented May 15, 2022

relates to: #23538
closes: #23538


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragement file, named {pr_number}.significant.rst, in newsfragments.

@Bowrna Bowrna closed this May 15, 2022
@Bowrna Bowrna force-pushed the ci_use_breeze_test branch from a4ad59b to 694e380 Compare May 15, 2022 11:08
@Bowrna Bowrna reopened this May 15, 2022
@Bowrna Bowrna force-pushed the ci_use_breeze_test branch 4 times, most recently from 9be5fe1 to b5fe319 Compare May 17, 2022 10:18
@Bowrna Bowrna marked this pull request as ready for review May 17, 2022 10:18
Copy link
Contributor

@joppevos joppevos left a comment

Choose a reason for hiding this comment

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

Nice work!

@potiuk
Copy link
Member

potiuk commented May 18, 2022

I think it needs quite a bit more changes :).

More details in #23538 (comment)

@Bowrna Bowrna force-pushed the ci_use_breeze_test branch 2 times, most recently from fdb2de5 to 537c58c Compare June 1, 2022 07:35
@Bowrna Bowrna force-pushed the ci_use_breeze_test branch 2 times, most recently from da3abda to c489755 Compare June 9, 2022 08:16
@potiuk
Copy link
Member

potiuk commented Jun 14, 2022

Hey @Bowrna the #24236 is merged - feel free to rebase and work on "parallelising" :)

@Bowrna Bowrna force-pushed the ci_use_breeze_test branch 2 times, most recently from e67d8af to 1436ce7 Compare June 20, 2022 04:04
@@ -313,6 +369,30 @@ def tests(
verbose=verbose,
dry_run=dry_run,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@potiuk I have rebased the changes in main and currently, I have changes to run the test in parallel and the changes you have added include showing the limited progress.

I need to add the option to show the limited progress in parallel tests alone or both parallel and usual test flow?

Copy link
Member

Choose a reason for hiding this comment

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

I think the "limited" output should be always enabled when parallell is used

Copy link
Member

Choose a reason for hiding this comment

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

I just opened a PR to remove the option in Helm tests here: #26144

@Bowrna Bowrna force-pushed the ci_use_breeze_test branch from 1436ce7 to 22c71e5 Compare June 29, 2022 10:17
@eladkal
Copy link
Contributor

eladkal commented Jul 20, 2022

@Bowrna there are conflicts to resolve

image_tag: Optional[str],
mount_sources: str,
skip_cleanup: bool,
include_success_outputs: bool,
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove limit_progress_output parameter altogether (from both helm_tests and testing). Once we get parallel running, the limit_progess_output should be always enabled when run in parallel and disabled when not, so we do not need another flag there.

@Bowrna Bowrna force-pushed the ci_use_breeze_test branch from 013e9d1 to b20fc44 Compare September 3, 2022 08:46
@potiuk
Copy link
Member

potiuk commented Sep 3, 2022

Hey @Bowrna - see https://github.com/apache/airflow/actions/runs/2983790915 :

When you have a bug in the workflow - you will see exclamation mark and detailed explanation :

Invalid workflow file: .github/workflows/ci.yml#L1007
The workflow is not valid. .github/workflows/ci.yml (Line: 1007, Col: 9): 'run' is already defined .github/workflows/ci.yml (Line: 1008, Col: 9): 'env' is already defined

@Bowrna Bowrna force-pushed the ci_use_breeze_test branch 6 times, most recently from fe96f3c to bc42ae8 Compare September 3, 2022 16:21
@Bowrna Bowrna closed this Sep 5, 2022
@Bowrna Bowrna reopened this Sep 5, 2022
@Bowrna Bowrna force-pushed the ci_use_breeze_test branch 3 times, most recently from 06ec88e to b4e088a Compare September 6, 2022 09:42
@Bowrna
Copy link
Contributor Author

Bowrna commented Sep 6, 2022

@potiuk I am facing issues in CI workflow in breeze parallel tests
https://github.com/apache/airflow/runs/8204814169?check_suite_focus=true

But I am not sure why it's failing and can't figure out much from the error logs. Can you help me how I could understand this part better?

I tried running it on my machine and it failed in different steps with error below

Bind for 0.0.0.0:28080 failed: port is already allocated

@Bowrna Bowrna force-pushed the ci_use_breeze_test branch from b4e088a to 3780dc8 Compare September 6, 2022 15:55
@Bowrna
Copy link
Contributor Author

Bowrna commented Sep 10, 2022

@potiuk I am facing issues in CI workflow in breeze parallel tests https://github.com/apache/airflow/runs/8204814169?check_suite_focus=true

But I am not sure why it's failing and can't figure out much from the error logs. Can you help me how I could understand this part better?

I tried running it on my machine and it failed in different steps with error below

Bind for 0.0.0.0:28080 failed: port is already allocated

@potiuk when you get time, please give some directions in resolving this issue. Thanks

@potiuk
Copy link
Member

potiuk commented Sep 18, 2022

@potiuk when you get time, please give some directions in resolving this issue. Thanks

The thing here is that you should override (like it is in the original code) the port number for each "tests" command. Each of the tests run a command which is run here needs a different port number when run in paralllel and in the original code I am dynamically generating the port number so that they are different. This is a bit "arcane" so if you feel like this might be too difficult, I can take over that one?

@Bowrna
Copy link
Contributor Author

Bowrna commented Sep 19, 2022

@potiuk when you get time, please give some directions in resolving this issue. Thanks

The thing here is that you should override (like it is in the original code) the port number for each "tests" command. Each of the tests run a command which is run here needs a different port number when run in paralllel and in the original code I am dynamically generating the port number so that they are different. This is a bit "arcane" so if you feel like this might be too difficult, I can take over that one?

Yes @potiuk I think you can take over that part and I will watch how you are doing the dynamic allocation of port number. So that I can take over something like that next time and do it

This reverts commit 516c17cf252b332be6d2d886d38c6a289cb9bbf1.
@potiuk
Copy link
Member

potiuk commented Sep 28, 2022

I think #26612 will likely get it obsolete :).
I've done much more there - like debugging and tracking what's going on + some optimisations. It's close to merge, just testing last case. But it turned out to be quite a bit more complex eventually

@Bowrna
Copy link
Contributor Author

Bowrna commented Sep 28, 2022

@potiuk Oh, I see! I didn't notice the changes you are making in that PR. Let me check it now.

@eladkal
Copy link
Contributor

eladkal commented Nov 11, 2022

@Bowrna do we still need this PR?

@potiuk
Copy link
Member

potiuk commented Nov 11, 2022

Ah no. It's implemented already :)

@potiuk potiuk closed this Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: convert tests to use Breeze tests command
5 participants