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

Rename worker_spec attribute #282

Merged
merged 11 commits into from Jun 30, 2019

Conversation

@ocaisa
Copy link
Contributor

ocaisa commented Jun 26, 2019

Short-term fix for #280

For tests to pass, requires #284

@ocaisa

This comment has been minimized.

Copy link
Contributor Author

ocaisa commented Jun 26, 2019

Closing and reopening to see if that affects tests passing

@ocaisa ocaisa closed this Jun 26, 2019
@ocaisa ocaisa reopened this Jun 26, 2019
@danpf

This comment has been minimized.

Copy link
Contributor

danpf commented Jun 26, 2019

thank you for doing this @ocaisa !!!

one of your problems is that ncores is now nthreads

I'm not really sure what the other problems could be. some of the complaints are about AttributeErrors but that should be caught by:
https://github.com/dask/distributed/blob/master/distributed/deploy/adaptive.py#L216
(and the line numbers in the error match so it should be okay?)

ocaisa added 2 commits Jun 26, 2019
@danpf

This comment has been minimized.

Copy link
Contributor

danpf commented Jun 26, 2019

you can revert that last change, requirements.txt isn't respected :(
it's built by conda and just uses the newest (in this case 2.0)
https://github.com/dask/dask-jobqueue/blob/master/ci/slurm/Dockerfile#L8

I would suggest giving JobQueuePlugin a workers property that returns self.scheduler.workers

I don't actually know if that is the answer, but digging deeper into the logs

Traceback (most recent call last):
  File "/opt/anaconda/lib/python3.6/site-packages/tornado/ioloop.py", line 743, in _run_callback
    ret = callback()
  File "/opt/anaconda/lib/python3.6/site-packages/tornado/ioloop.py", line 767, in _discard_future_result
    future.result()
  File "/opt/anaconda/lib/python3.6/site-packages/tornado/gen.py", line 209, in wrapper
    yielded = next(result)
  File "/opt/anaconda/lib/python3.6/site-packages/distributed/deploy/adaptive.py", line 259, in _adapt
    recommendations = self.recommendations()
  File "/opt/anaconda/lib/python3.6/site-packages/distributed/deploy/adaptive.py", line 218, in recommendations
    current = len(self.cluster.workers)
AttributeError: 'SLURMCluster' object has no attribute 'workers'
tornado.application - ERROR - Exception in callback functools.partial(<bound method IOLoop._discard_future_result of <tornado.platform.asyncio.AsyncIOLoop object at 0x7f81de0b95f8>>, <Future finished exception=AttributeError("'SLURMCluster' object has no attribute 'workers'",)>)
Traceback (most recent call last):
  File "/opt/anaconda/lib/python3.6/site-packages/distributed/deploy/adaptive.py", line 216, in recommendations
    current = len(self.cluster.worker_spec)
AttributeError: 'SLURMCluster' object has no attribute 'worker_spec'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/opt/anaconda/lib/python3.6/site-packages/tornado/ioloop.py", line 743, in _run_callback
    ret = callback()
  File "/opt/anaconda/lib/python3.6/site-packages/tornado/ioloop.py", line 767, in _discard_future_result
    future.result()
  File "/opt/anaconda/lib/python3.6/site-packages/tornado/gen.py", line 209, in wrapper
    yielded = next(result)
  File "/opt/anaconda/lib/python3.6/site-packages/distributed/deploy/adaptive.py", line 259, in _adapt
    recommendations = self.recommendations()
  File "/opt/anaconda/lib/python3.6/site-packages/distributed/deploy/adaptive.py", line 218, in recommendations
    current = len(self.cluster.workers)
AttributeError: 'SLURMCluster' object has no attribute 'workers'

that's probably at least one of the problems

if you have a better idea feel free to try that instead, i think that should get us in the right direction though

@ocaisa

This comment has been minimized.

Copy link
Contributor Author

ocaisa commented Jun 27, 2019

Thanks @danpf , that suggestion worked. The last issue is that when using Python2, we still havencores rather than nthreads, I can make this work with an if/else to get this set of tests to pass but it's probably an indicator of a necessary break with Python2 support.

@willirath

This comment has been minimized.

Copy link
Collaborator

willirath commented Jun 27, 2019

Dask has dropped Python 2 with v2.0.0:

In my opinion, this should be reflected here (the repo) as well.

@guillaumeeb

This comment has been minimized.

Copy link
Member

guillaumeeb commented Jun 27, 2019

I agree with @willirath on the Python 2 support.

I'll try to review the PR next week, but if someone else (@danpf @willirath) can do it before, I'll be quite happy!

dask_jobqueue/tests/test_sge.py Outdated Show resolved Hide resolved
Copy link
Contributor

danpf left a comment

Everything you've done looks great!

to remove testing from python2 you must remove the python2 testing blocks from:
https://github.com/dask/dask-jobqueue/blob/master/.travis.yml

I would say add python_requires='>3.5.0', to the setup.py, but I don't know if that's sufficient to stop it from being served to python2.7 pip/conda

@ocaisa

This comment has been minimized.

Copy link
Contributor Author

ocaisa commented Jun 28, 2019

I looked at what was done in distributed (which is what you suggested) and, given the behaviour we've seen in the testing here, it looks like that is enough to stop serving to python2.

@danpf

This comment has been minimized.

Copy link
Contributor

danpf commented Jun 28, 2019

Great! thanks @ocaisa & nice job! everything looks good to me!

@ocaisa ocaisa changed the title Rename worker_spec attribute Drop Python 2 support and rename worker_spec attribute Jun 28, 2019
@willirath

This comment has been minimized.

Copy link
Collaborator

willirath commented Jun 28, 2019

Looks good to me as well. The pedant in me tells me, however, that this should be two PR's:

  1. Drop Python 2 testing. Should also add a clear statement about dropping 2 to the https://github.com/dask/dask-jobqueue/blob/master/docs/source/changelog.rst

  2. Fix #280

@ocaisa

This comment has been minimized.

Copy link
Contributor Author

ocaisa commented Jun 28, 2019

@willirath I agree you with there, something like dropping python2 support deserves a distinct PR. Will open now.

ocaisa added 2 commits Jun 28, 2019
@ocaisa ocaisa changed the title Drop Python 2 support and rename worker_spec attribute Rename worker_spec attribute Jun 28, 2019
@willirath willirath referenced this pull request Jun 28, 2019
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@willirath

This comment has been minimized.

Copy link
Collaborator

willirath commented Jun 28, 2019

@ocaisa Sorry again for the confusion. The proposed separation was not so much about the importance or impact of dropping Python 2 warranting a separate PR, but about keeping a clean history that separates commits to master when different purposes are served. I have tried to clarify this in the reviews of #282 and #283 now, I hope:

#282 just fixes worker_spec and disregards red marks on Py2 related tests for now.

#283 drops Python 2 testing but doesn't touch anything else.

@ocaisa ocaisa referenced this pull request Jun 28, 2019
Copy link
Member

guillaumeeb left a comment

Thanks a lot @ocaisa!

This is good to make dask-jobqueue compatible with distributed again!
We'll need to work on making it well integrated with SpecificationCluster though!

@guillaumeeb guillaumeeb merged commit bff96a3 into dask:master Jun 30, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@ocaisa ocaisa deleted the ocaisa:jq_worker_spec branch Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.