Fixed dask executor and tests #22027
Conversation
… tests folder and the certificates, added certificates to certs folder
boring-cyborg
bot
added
the
area:Scheduler
| @@ -96,7 +96,7 @@ def airflow_run(): | |||
| raise AirflowException(f"Attempted to submit task to an unavailable queue: '{queue}'") | |||
| resources = {queue: 1} | |||
|
|
|||
| future = self.client.submit(airflow_run, pure=False, resources=resources) | |||
| future = self.client.submit(subprocess.check_call, command, pure=False, resources=resources) | |||
subkanthi
changed the title
|
Ah, nice. If we can make it works that would be cool :). |
| # These use test certs that ship with dask/distributed and should not be | ||
| # used in production | ||
| with dask_testing_cluster( | ||
| worker_kwargs={'security': tls_security(), "protocol": "tls"}, |
There was a problem hiding this comment.
tls_security in utils_test.py part of distributed seems to look for the certs in the tests/ path
path = os.path.join(certs_dir, filename)
For now commenting out if its OK, the other tests seem to pass
There was a problem hiding this comment.
Let's see. They did not for me when I run them previously.
There was a problem hiding this comment.
Atleast on my local, most of the tests were failing because of errors with pickling the function, the change in [airflow/executors/dask_executor.py], fixed it
- future = self.client.submit(airflow_run, pure=False, resources=resources)
+ future = self.client.submit(subprocess.check_call, command, pure=False, resources=resources)
There was a problem hiding this comment.
Cool @subkanthi ! Looks good. I think you can bring back the tests now - in my change (when you rebase) you will see how I dealt with get_cert to skip those
There was a problem hiding this comment.
I think you cna ucomment your line comments now :).
|
I think the problem is that constraints have not been refreshed yet. Let's try tomorrow. |
|
Can you please rebase @subkanthi - I have just updated the constraints. |
subkanthi
changed the title
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
github-actions
bot
added
the
full tests needed
|
Wohooo!! Thanks @subkanthi ! |
Fixed dask executor and tests, distributed package does not ship with tests folder and the certificates, added certificates to certs folder (cherry picked from commit d3c168c)
Fixed dask executor and tests, distributed package does not ship with tests folder and the certificates, added certificates to certs folder (cherry picked from commit d3c168c)
Fixed dask executor and tests, distributed package does not ship with tests folder and the certificates, added certificates to certs folder (cherry picked from commit d3c168c)
Fixed dask executor and tests, distributed package does not ship with tests folder and the certificates, added certificates to certs folder (cherry picked from commit d3c168c)
Fixed dask executor and tests.
Distributed pip package does not ship with tests folder anymore.
And there is a python script in distributed/tests to create the certificates.
Two tests are still failing, test_tls and test_backfill_integration.
The certificates cause a spelling error, not sure if we really need this test where certs are bundled.
related: #22017
^ 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.