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.

Sign up for GitHub

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

Jump to bottom

[WIP] Attempt to make job submission async #473

Closed
wants to merge 3 commits into from
Closed

[WIP] Attempt to make job submission async #473

wants to merge 3 commits into from

Conversation

oshadura
Copy link

Probably fixes: #470
cc: @bbockelm

@oshadura oshadura mentioned this pull request Nov 13, 2020
Closed
mivade
mivade reviewed Nov 13, 2020
View reviewed changes
dask_jobqueue/core.py Outdated
# Should we make this async friendly?
return self._call(shlex.split(self.submit_command) + [script_filename])
call = shlex.split(self.submit_command + [script_filename])
return await asyncio.get_running_loop().run_in_executor(None, self._call(call))
Copy link

Choose a reason for hiding this comment

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

If I'm reading this right, I don't think there's a need to run this in an executor as you already made _call async.

@lesteve
Copy link
Member

lesteve commented Nov 17, 2020

Some CI error looks genuine for example this one: https://github.com/dask/dask-jobqueue/pull/473/checks?check_run_id=1406501917.

I am not an async expert, but I would guess you need to add some await where _call is called, maybe changing the functions calling _call to async. One such function seems to be _job_id_from_submit_output, there may be others. In an ideal world, the main user-facing functions would stay synchronous so that if you get an exception it blows up in an obvious way rather than having some log message saying that an exception was never retrieved.

Full disclosure : unfortunately I don't really understand the original issue #470 ...

@lesteve lesteve force-pushed the master branch 2 times, most recently from 4d181fe to 26a0e70 Compare December 8, 2020 12:40
@andersy005 andersy005 added all job schedulers enhancement New feature or request labels Jan 24, 2021
Base automatically changed from master to main February 10, 2021 07:12
@guillaumeeb
Copy link
Member

This PR is a bit old... @oshadura are you still around and up for this change?

I'm going to close this one next time I'll take a look at the activity here, unless someone updates it.

@oshadura
Copy link
Author

oshadura commented Sep 1, 2021 •
edited
Loading

@guillaumeeb sorry I was working on other things and thanks for reminder! I will definitely try to take a look in next couple of days and update/close PR.

@jrueb jrueb mentioned this pull request Jul 12, 2023
Merged
@jacobtomlinson
Copy link
Member

It's been a long time since this PR had any activity so I'm going to close it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mivade mivade mivade left review comments

Assignees
No one assigned
Labels
all job schedulers enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make job submission asynchronous
6 participants
@oshadura @lesteve @guillaumeeb @jacobtomlinson @mivade @andersy005