Not setting `project` should not raise

[bocklund]

One suggestion I have is not to raise an exception unless there is exceptional behavior. In the case of not having a project set, not all systems require users to set a project, so we should not enforce that by raising IMO. This would also apply to #6

[mrocklin]

If people are ok with this idea would you be interested in submitting a PR to resolve this?

…

On Fri, Mar 2, 2018 at 5:57 AM, Brandon Bocklund ***@***.***> wrote: One suggestion I have is not to raise an exception unless there is exceptional behavior. In the case of not having a project set, not all systems require users to set a project, so we should not enforce that by raising IMO. This would also apply to #6 <#6> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#7>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASszCKXv7MNDF2PSnWibEKVfPkhW_-hks5taSWGgaJpZM4SZupT> .

[bocklund]

I'd be happy to help. distributed Clients running on HPC resources is a core use case for us over at ESPEI

[jhamman]

I think we could take some time to rethink the specification of cluster specific configuration options (project included). I've not worked on two computers that use identical syntax for the batch script headers so this is something we're going to run into repeatedly.

[jhamman]

for the sake of argument, what if we changed the current cluster signature from:

PBSCluster(name='dask',
           queue='regular',
           project=None,
           resource_spec='select=1:ncpus=36:mem=109GB',
           threads_per_worker=4,
           processes=9,
           memory='7GB',
           walltime='00:30:00',
           interface=None,
           death_timeout=60,
           extra='',
           **kwargs)

to something more flexible like:

cluster_kws = {}
job_args =  ['-l select=1:ncpus=36:mem=109GB', '-l walltime=00:30:00',
                 '-q regular', 'A PROJECT']
worker_kws = dict(threads_per_worker=4, processes=9, memory='7GB',
                  interface=None, death_timeout=60, extra='',)

PBSCluster(name='dask', cluster_args=cluster_args, job_args=job_args,
           worker_kws=worker_kws)

where the job script is created programmatically:

header = ['#!/bin/bash', '#PBS -N %(name)s' % name]
header.extend(['#PBS %(arg)s' for arg in cluster_args])
header = '\n'.join(header)

[mrocklin]

I agree that this might provide a valuable escape hatch for unique cluster situations. I am slightly concerned that people are no longer prompted with options like wall_time=, threads_per_worker= etc, that they might use if presented with the option. I wonder if we can do a mix of explicit keywords alongside catch-all dictionary keywords.

[guillaumeeb]

From my point of view, it would be better to keep explicit keyword at least for basic PBS or Dask options.
And as @mrocklin said, mixing it with dictionary keywords or lists as proposed by @jhamman seems really good.
I would also suggest to explicitly tell wether this is PBS or Dask keyword, so something like that:

PBSCluster(pbs_name='dask', pbs_resource_spec=None, pbs_walltime='00:30:00',
    pbs_queue=None, pbs_project=None, pbs_extra=[], 
    dask_worker_threads=1, dask_worker_processes=1, dask_worker_memory='4GB', 
    dask_interface=None, dask_worker_death_timeout=60, dask_extra='')

I feel that already proposed arguments are perfect, and the most used ones. In our cluster, we have a default for all PBS options, but the usual minimal options to set are the job name and the walltime. Since we are talking about starting a cluster here, we can add to that the resource_spec. I would almost suggest to create 3 args from the PBS resource_spec arg:

pbs_chunk_nbr=1 # or pbs_select=1
pbs_chunk_ncpus=36
pbs_chunk_mem='109GB'

But it is maybe overkill.

I was searching if there was a need for dask_scheduler options, but as I understand from code (Did not try it yet), the scheduler is launched directly from where the PBSCluster is created, using a Localcluster. So this means either on a interactive or frontal cluster node, or directly in a notebook. I guess this is OK regarding the resource footprint of the scheduler (CPU, RAM)? What I mean is, we do not want to spawn a specific qsub job for the scheduler?

For information, see below PBS complete option list, I don't think there is anything else needed except in specific cases.

qsub [-a date_time] [-A account_string] [-c interval]
            [-C directive_prefix] [-e path] [-f] [-h]
            [-I [-G [-- <GUI application>]] | [-X]] [-j join] [-J range] [-k keep] [-l resource_list] [-m mail_events] [-M user_list] [-N name] [-o path] [-p priority] [-P project] [-q destination] [-r
            c] [-S path_list] [-u user_list] [-v variable_list] [-V] [-W additional_attributes] [-z] [script | -- executable [arglist for executable]]

[guillaumeeb]

Hm, it just occured to me that every PBS options given was for the launch of only one worker... I know, I should have RTFM, at least better. But maybe it should be more transparent from the keywords name?
This does not change a lot from what I wrote above, maybe the resource_spec default value should be set (and a little lower that what is currently proposed), for example select=1:ncpus=2:mem=10GB. And we shall never have selected chunk to more than 1.

Also, maybe we should provide a n_workers keyword directly in the PBSCluster constructor, as with LocalCluster, or KubeCluster? And start right away the cluster? I don't have a strong point on that, it would just be more consistent with other Dask provided cluster.

[jhamman]

I'm also fine with a more fine grained approach (as mentioned by @guillaumeeb). The thing we really need here is the ability to construct the header programatically. That should allow us to drop the JobQueueCluster._template attribute all together. This is not something I'm likely to get to in the short term so if anyone else wants to take a swing at it, that would be great.

[guillaumeeb]

So if I understand correctly, what you really want to avoid is to have fixed header options as it is now, and thus m=be more generic to cluster specific needs. So instead of using the _template, with a fine grained approach, the PBSCluster.__init__ would look something like this:

header = ['#!/bin/bash', '#PBS -N %(name)s' % name]
if queue != None:
    header.append('#PBS -q %(queue)s' % queue)
if project != None:
    header.append('#PBS -A %(project)s' % project)
...
header.extend(['#PBS %(arg)s' for arg in pbs_extra])
self._script = '\n'.join(header)

#build dask worker comand and add it to self._script
...

And then we should modify the JobQueueCluster.job_script() function, to only modify the worker index in the _script attribute.

[guillaumeeb]

@mrocklin @jhamman, if you confirm you expect something like thiw, I could work on this next week. It would probably has an effect on core class and every scheduler impl though.

[jhamman]

@guillaumeeb - yes, your example above is more or less what I was thinking of. There are probably many ways to do this but I think you have the basic idea of what I was shooting for.

[mrocklin]

I would like to avoid prefixes like pbs_*. These seem unnecessary given PBS in the name of the class.

[guillaumeeb]

That's fine with me, I will work on a pull request on this basis. Thanks for the comments.