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

Use host= keyword rather than ip= #278

Merged
merged 5 commits into from Jul 23, 2019

Conversation

@mrocklin
Copy link
Member

mrocklin commented Jun 13, 2019

This will change in the next release. We should merge this and release after we next release dask/distributed.

@lesteve lesteve force-pushed the mrocklin:update-dask-2 branch from c13b3b5 to be89992 Jul 22, 2019
Requirements bump will be done in a separate PR.
@lesteve

This comment has been minimized.

Copy link
Member

lesteve commented Jul 22, 2019

@mrocklin do you think we should merge this one? My understanding:

  • ip is marked deprecated in LocalCluster docstring but using ip does not create a warning in distributed master yet.
  • there was a failure at the time you opened the PR (distributed 1.28.1) which seemed genuine (the test is supposed to check that by default the scheduler starts on an IP address that is reachable from other nodes). This seems fixed in distributed 2.1 although I was not able to find the related entry in the distributed changelog.

More details: this PR was failing when @mrocklin originally opened the PR (June 13), see this Travis log. The error was:

_______________________________ test_forward_ip ________________________________
    def test_forward_ip():
        ip = "127.0.0.1"
        with PBSCluster(
            walltime="00:02:00",
            processes=4,
            cores=8,
            memory="28GB",
            name="dask-worker",
            host=ip,
        ) as cluster:
            assert cluster.local_cluster.scheduler.ip == ip
    
        default_ip = socket.gethostbyname("")
        with PBSCluster(
            walltime="00:02:00", processes=4, cores=8, memory="28GB", name="dask-worker"
        ) as cluster:
>           assert cluster.local_cluster.scheduler.ip == default_ip
E           AssertionError: assert '127.0.0.1' == '0.0.0.0'
E             - 127.0.0.1
E             + 0.0.0.0
dask_jobqueue/tests/test_jobqueue_core.py:104: AssertionError
=============== 1 failed, 78 passed, 15 skipped in 2.92 seconds ================

I was able to reproduce the original error locally with distributed=1.28.1.

@mrocklin

This comment has been minimized.

Copy link
Member Author

mrocklin commented Jul 22, 2019

ip is marked deprecated in LocalCluster docstring but using ip does not create a warning in distributed master yet.

Right. At one point in development we were adding a warning if the user used ip=, but then after submitting this PR I decided that that was probably too aggressive, particularly because this would effectively force dask-jobqueue to drop Python 2 going forward (dask 2.0 drops the ip= keyword and Python 2, so dask-jobqueue would have to either accept annoying warnings, or drop Python 2 as well). I thought that HPC users are probably a little bit less able to move to Python 3 and so there wasn't a rush.

I would be happy to merge this any time if you like. I'll leave it to you.

@lesteve

This comment has been minimized.

Copy link
Member

lesteve commented Jul 22, 2019

We dropped Python 2 support for 0.6 (roughly 2 weeks ago). 0.6 was supposed to be compatible with dask >= 2 but it looks like we missed a few things, which I am trying to fix in the next release.

About dropping Python 2 support for HPC users, all of the people around me use Python 3 already with a conda installed as a user on a shared network drive. If you want to bring in other people that may have another experience from HPC setup into the conversation, you would be more than welcome to do so!

@mrocklin

This comment has been minimized.

Copy link
Member Author

mrocklin commented Jul 22, 2019

@lesteve

This comment has been minimized.

Copy link
Member

lesteve commented Jul 23, 2019

Fine, let's merge this one then!

@lesteve lesteve merged commit ca7ec4f into dask:master Jul 23, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lesteve

This comment has been minimized.

Copy link
Member

lesteve commented Jul 23, 2019

Thanks @mrocklin!

@lesteve

This comment has been minimized.

Copy link
Member

lesteve commented Jul 23, 2019

0.6 was supposed to be compatible with dask >= 2 but it looks like we missed a few things, which I am trying to fix in the next release.

BTW @guillaumeeb or @mrocklin if you could give me the rights on PyPI, that would be great! My user name on PyPI is lesteve as well.

@mrocklin

This comment has been minimized.

Copy link
Member Author

mrocklin commented Jul 23, 2019

@lesteve

This comment has been minimized.

Copy link
Member

lesteve commented Jul 23, 2019

Great thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.