Skip to content

Fix jobqueue system config_name not correctly passed through#361

Closed
guillaumeeb wants to merge 2 commits into
dask:masterfrom
guillaumeeb:fix_interface_scheduler_address
Closed

Fix jobqueue system config_name not correctly passed through#361
guillaumeeb wants to merge 2 commits into
dask:masterfrom
guillaumeeb:fix_interface_scheduler_address

Conversation

@guillaumeeb

Copy link
Copy Markdown
Member

Closes #358

@guillaumeeb

Copy link
Copy Markdown
Member Author

I tried to think how to test this, but nothing really clean comes to my mind, only something involving checking cluster._kwargs to check interface ... Should we do that?

@mrocklin

Copy link
Copy Markdown
Member

Thanks for resolving this @guillaumeeb .

For testing, maybe we should check the scheduler_spec or worker_spec, to verify that the interface is somewhere in there?

async def test_foo(cleanup);
    with dask.config.set(...):
        cluster = PBSCluster(asynchronous=True)  # no need to await it
        assert "my-interface" in str(cluster.scheduler_spec)
        assert "my-interface" in str(cluster.worker_spec)

@guillaumeeb

Copy link
Copy Markdown
Member Author

Thanks for the hint @mrocklin, I tried to add something. Did not pu the second assert because it caused problems as cluster.worker_spec was None and also because the original problem arised in Scheduler spec.

Comment thread dask_jobqueue/core.py

if config_name:
if config_name is not None:
self.config_name = config_name

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not convinced this is the right fix, not 100% sure though. I need to look into it more ...

My instincts is that there is some unnecessary complicated things with config_name both a class variable at the Cluster level and the Job level ... a bit vague but I'll try to look at it tomorrow in more details.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah I agree this might thought about it a little more, but maybe we could do this after fixing this problem? (I know, we'll probably never look at it again ...)

Maybe we should merge this and open another issue to indicate that we parse the config twice and create duplicate class variables?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wow it has been 19 days already ... I have a WIP PR in #366. Let me know if you have any comments.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Interface not loaded from config file

3 participants