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

Removes limitations from Dask dependencies #22017

Merged
merged 1 commit into from
Mar 6, 2022

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Mar 5, 2022

Dask dependencies were holding us back - when it comes to upgrading
somoe of the packages (for example apache-beam and looker - in google
provider). This PR removes the limitations but with a twist.

  • Dask tests stop working. We reach out to the Dask Team to fix them
    but since a very old version of distributed library was used
    the Dask team is called for help to fix those

  • The typing-extensions library was limited by distributed but it
    seems that version 4.0.0+ breaks kubernetes tests


^ Add meaningful description above

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

@boring-cyborg boring-cyborg bot added the area:Scheduler including HA (high availability) scheduler label Mar 5, 2022
Dask dependencies were holding us back - when it comes to upgrading
somoe of the packages (for example apache-beam and looker - in google
provider). This PR removes the limitations but with a twist.

* Dask tests stop working. We reach out to the Dask Team to fix them
  but since a very old version of `distributed` library was used
  the Dask team is called for help to fix those

* The typing-extensions library was limited by `distributed` but it
  seems that version 4.0.0+ breaks kubernetes tests
@potiuk potiuk force-pushed the remove-limitations-from-dask branch from 361621e to b7e4b5f Compare March 5, 2022 21:15
@potiuk potiuk added the full tests needed We need to run full set of tests for this PR to merge label Mar 5, 2022
@potiuk potiuk closed this Mar 5, 2022
@potiuk potiuk reopened this Mar 5, 2022
@potiuk
Copy link
Member Author

potiuk commented Mar 5, 2022

cc: @alekseiloginov - related to #20882


DEFAULT_DATE = timezone.datetime(2017, 1, 1)
SUCCESS_COMMAND = ['airflow', 'tasks', 'run', '--help']
FAIL_COMMAND = ['airflow', 'tasks', 'run', 'false']

# For now we are temporarily removing Dask support until we get Dask Team help us in making the
# tests pass again
skip_dask_tests = True
Copy link
Contributor

Choose a reason for hiding this comment

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

@potiuk , Out of curiosity, is this strictly maintained by the Dask team, is this something I can take a look, I was looking also at options of connecting to a Local Dask cluster along with distributed.

Copy link
Member Author

@potiuk potiuk Mar 5, 2022

Choose a reason for hiding this comment

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

It is not. I started discussion on that here https://lists.apache.org/thread/6stgcpjt5jb3xfw92oo1j486j33c8v7m

This is is a second time I start similar discussion - the previous time it was in Jan 2020 https://lists.apache.org/thread/875fpgb7vfpmtxrmt19jmo8d3p6mgqnh and then Dask team chimed in and helped in fixing the tests.

But more than 1 year later we have similar problem.

I also asked at Dask's disccoure whether they can help again: https://dask.discourse.group/t/potential-removal-of-dask-executor-support-in-airflow/433

@potiuk
Copy link
Member Author

potiuk commented Mar 6, 2022

All looks good now with those changes. I propose to merge it so that we are unblocked while we discuss next steps.

@potiuk potiuk requested review from eladkal and malthe March 6, 2022 19:37
@malthe
Copy link
Contributor

malthe commented Mar 6, 2022

@potiuk isn't it possible (and easier) to skip these tests "from the outside" – ?

I would think skipif is more suitable for a situation where you want to skip a test depending on for example the operating system.

@potiuk
Copy link
Member Author

potiuk commented Mar 6, 2022

@potiuk isn't it possible (and easier) to skip these tests "from the outside" – ?

I would think skipif is more suitable for a situation where you want to skip a test depending on for example the operating system.

How would you imagine "externall" exclusion :)?

I would like to avoid having anyone to add extra pytest arguments or command line switches to skip those. The idea is that whoever runs "All tests" for Airlflow can see "blessed tests" succeed, no matter if they specify some exclusions.

In our case, if you want to make sure that all tests are succeeding you do this:

  • ./breeze build-image --python 3.7
  • ./breeze --backend mysql
  • while in "breeze" you run pytest tests

Those three commands should lead to successful test execution (in 3.7 + mysql combination).

Having to add some additional exclusion rules is bad, especially in case we want Dask team members to help with fixing those. What we ask them, is to eventually remove skipIf (initially set the `skip_dask_test' to False and run the failed tests). They do not have to learn anything about external scripts and "Exclusions" we do outside of our "python" code. their fix will be limited to "dask" tests only - they don't have to understand what our CI does and how we exclude stuff externally.

I think in this case skipIf "close" to the tests being disabled is exactly what we need.


@pytest.mark.skipif(skip_dask_tests, reason="The tests are skipped because it needs testing from Dask team")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@pytest.mark.skipif(skip_dask_tests, reason="The tests are skipped because it needs testing from Dask team")
@pytest.mark.skip(reason="The tests are skipped because it needs testing from Dask team")

skipif is used to dynamically skip tests depending on a dynamic value, e.g. in the active database backend.

Copy link
Member Author

@potiuk potiuk Mar 6, 2022

Choose a reason for hiding this comment

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

I wanted to give an easy way for Dask committers to be able to "play" with it. it's easier to set up one variable to True to do it, rather than manually remove @pytest.mark.skip for all test cases.

This is really a "temporary" state I wanted to make easy for them to work on (same as last time).

Copy link
Member Author

Choose a reason for hiding this comment

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

I imagine the Dask developers will do it this way:

  1. setup Breeze
  2. get Dask service to run tests on
  3. set skip_dag_tests to False
  4. fix the tests
  5. remove skips

By having single flag to switch that enables all tests it is just easier to not forget about removing some of the skips.

@potiuk potiuk mentioned this pull request Mar 6, 2022
2 tasks
@potiuk potiuk merged commit 9be3c50 into apache:main Mar 6, 2022
@potiuk potiuk deleted the remove-limitations-from-dask branch March 6, 2022 23:59
@potiuk
Copy link
Member Author

potiuk commented Mar 6, 2022

Merging for now. We can discuss futher steps later.

@jedcunningham jedcunningham added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Scheduler including HA (high availability) scheduler changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants