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

Improve error message when cores or memory is not specified #331

Merged
merged 9 commits into from Aug 29, 2019

Conversation

@rblcoder
Copy link
Contributor

@rblcoder rblcoder commented Aug 27, 2019

Hi,

Please let me know if the changes are fine.

Fix #329.

@lesteve
Copy link
Member

@lesteve lesteve commented Aug 28, 2019

Thanks a lot for the PR! This definitely goes in the right direction.

Could you add a test in dask_jobqueue/tests/test_jobqueue_core.py? Something similar to this (look at the tests in this file if you need inspiration):

@pytest.mark.parametrize(
    "Cluster",
    [PBSCluster, MoabCluster, SLURMCluster, SGECluster, LSFCluster, OARCluster],
)
def test_cluster_has_cores_and_memory(Cluster):
    with pytest.raises(ValueError, match='cores=.+memory='):
        with Cluster():
            pass

    with pytest.raises(ValueError, match='cores='):
        with Cluster(memory='1GB'):
            pass

    with pytest.raises(ValueError, match='memory='):
        with Cluster(cores=4):
            pass

@lesteve
Copy link
Member

@lesteve lesteve commented Aug 28, 2019

FYI: you need to use "Fix #issueNumber" in your PR description (and not in the PR title), this way the associated issue gets closed automatically when the PR is merged. For more details, look at this.

I edited your PR description for you.

@rblcoder
Copy link
Contributor Author

@rblcoder rblcoder commented Aug 28, 2019

Thanks a lot for the PR! This definitely goes in the right direction.

Could you add a test in dask_jobqueue/tests/test_jobqueue_core.py? Something similar to this (look at the tests in this file if you need inspiration):

@pytest.mark.parametrize(
    "Cluster",
    [PBSCluster, MoabCluster, SLURMCluster, SGECluster, LSFCluster, OARCluster],
)
def test_cluster_has_cores_and_memory(Cluster):
    with pytest.raises(ValueError, match='cores=.+memory='):
        with Cluster():
            pass

    with pytest.raises(ValueError, match='cores='):
        with Cluster(memory='1GB'):
            pass

    with pytest.raises(ValueError, match='memory='):
        with Cluster(cores=4):
            pass

@lesteve I have added this test, please check if it is fine.

@rblcoder
Copy link
Contributor Author

@rblcoder rblcoder commented Aug 28, 2019

FYI: you need to use "Fix #issueNumber" in your PR description (and not in the PR title), this way the associated issue gets closed automatically when the PR is merged. For more details, look at this.

I edited your PR description for you.

@lesteve Thank you, I will keep this in mind.

Copy link
Member

@lesteve lesteve left a comment

Thanks for the test, here are a few comments!

"You must specify how many cores to use per job like ``cores=8``"
+ "as well as how much memory to use per job like ``memory='24 GB'``"
)
with pytest.raises(ValueError, match=error_message_string):
Copy link
Member

@lesteve lesteve Aug 28, 2019

Choose a reason for hiding this comment

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

I would suggest using regexp as I hinted in my previous message. You don't want to check the exact full message but only parts of it. In case someone does a minor wording change of the error message, you'd like the test to still pass (in an ideal world).

@@ -203,12 +203,16 @@ def __init__(
if shebang is None:
shebang = dask.config.get("jobqueue.%s.shebang" % config_name)

if cores is None:
if (cores is None) and (memory is None):
Copy link
Member

@lesteve lesteve Aug 28, 2019

Choose a reason for hiding this comment

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

You don't need the parentheses: if cores is None and memory is None. It may look weird if you are used to language like C but when are used to Python removing parentheses feels less noisy.

@@ -203,12 +203,16 @@ def __init__(
if shebang is None:
shebang = dask.config.get("jobqueue.%s.shebang" % config_name)

if cores is None:
if (cores is None) and (memory is None):
raise ValueError(
"You must specify how many cores to use per job like ``cores=8``"
Copy link
Member

@lesteve lesteve Aug 28, 2019

Choose a reason for hiding this comment

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

I would tweak the error messages slightly, e.g. something like (not tested):

"You must specify how much cores and memory per job you want to use, for example:\n"
"cluster = {}(cores={}, memory='{!r}'".format(self.__class__, cores or 8, memory or '24GB')

I would also use the same error message in all cases for simplicity reasons

@rblcoder
Copy link
Contributor Author

@rblcoder rblcoder commented Aug 29, 2019

Thank you, @lesteve. I have implemented changes according to your suggestions, please check.

@lesteve lesteve changed the title Improve error message when cores or memory is not specified fixes #329 Improve error message when cores or memory is not specified Aug 29, 2019
@lesteve
Copy link
Member

@lesteve lesteve commented Aug 29, 2019

Almost there: the error message is not quite what we want:

In [1]: from dask_jobqueue import SLURMCluster                                                                                                                                     

In [2]: cluster = SLURMCluster()                                                                                                                                                   
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-2-7bfc5f2e246f> in <module>
----> 1 cluster = SLURMCluster()

/home/local/lesteve/dev/dask-jobqueue/dask_jobqueue/slurm.py in __init__(self, queue, project, walltime, job_cpu, job_mem, job_extra, config_name, **kwargs)
     77             job_extra = dask.config.get("jobqueue.%s.job-extra" % config_name)
     78 
---> 79         super().__init__(config_name=config_name, **kwargs)
     80 
     81         # Always ask for only one task

/home/local/lesteve/dev/dask-jobqueue/dask_jobqueue/core.py in __init__(self, name, cores, memory, processes, interface, death_timeout, local_directory, extra, env_extra, log_directory, shebang, python, config_name, **kwargs)
    208                 "You must specify how much cores and memory per job you want to use, for example:\n"
    209                 "cluster = {}(cores={}, memory='{!r}'".format(
--> 210                     self.__class__.__name__, "cores or 8", "memory or '24GB'"
    211                 )
    212             )

ValueError: You must specify how much cores and memory per job you want to use, for example:
cluster = SLURMCluster(cores=cores or 8, memory='"memory or '24GB'"'

We want to recommend people to use this instead:

cluster = SLURMCluster(cores=8, memory='24GB')

@rblcoder
Copy link
Contributor Author

@rblcoder rblcoder commented Aug 29, 2019

Thank you, @lesteve. I have made that change.

@lesteve
Copy link
Member

@lesteve lesteve commented Aug 29, 2019

I pushed some tweaks but for some reason the CI has not been triggered, not sure why ...

@lesteve
Copy link
Member

@lesteve lesteve commented Aug 29, 2019

@rblcoder you should be able to trigger the CI again by pushing an empty commit.

@lesteve
Copy link
Member

@lesteve lesteve commented Aug 29, 2019

If the CI is green, I think this is fine to merge this one!

@lesteve
Copy link
Member

@lesteve lesteve commented Aug 29, 2019

I am going to merge this one since it looks like the CI eventually ran on my commit, thanks a lot!

@lesteve lesteve merged commit 975425e into dask:master Aug 29, 2019
1 check was pending
@rblcoder
Copy link
Contributor Author

@rblcoder rblcoder commented Aug 29, 2019

Thank you for your support @lesteve !

@rblcoder rblcoder deleted the error_message_cores_or_memory branch Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants