dask / dask-jobqueue

Deploy Dask on job schedulers like PBS, SLURM, and SGE
https://jobqueue.dask.org
BSD 3-Clause "New" or "Revised" License
235 stars 142 forks source link

rename `job_extra` #577

Closed jolange closed 2 years ago

jolange commented 2 years ago

to job_extra_directives, see #323.

If job_extra_directives is empty, job_extra will still be respected.

The warning code is duplicated for different *Cluster classes now. I thought about putting it in the base class, but that does not know this parameter (almost all implentations define it, but not all) and it is not treated exactly the same everywhere. So I think the repetition is ok for this temporary code.

guillaumeeb commented 2 years ago

Thanks @jolange for continuing work here, I really appreciate it.

The warning code is duplicated for different *Cluster classes now. I thought about putting it in the base class, but that does not know this parameter (almost all implentations define it, but not all) and it is not treated exactly the same everywhere

With a quick glance, I'm not sure why it is duplicated and not in the base class. Do you think something prevent to declare a private attribute in the main class, putting the warning in there, and just keep special handling in defferent implementations?

jolange commented 2 years ago

I did not check in detail before, which *Cluster classes do not implement it, but it now I see that it's actually only Moab and that this basically is PBS. So all of them use the parameter, really.

The only problem is that the parameter is supposed to be a dict for HTCondor, but a list for every other one.

guillaumeeb commented 2 years ago

The only problem is that the parameter is supposed to be a dict for HTCondor, but a list for every other one.

I guess this is a problem only for the doc string or warning message? Maybe we could indicate list or dict as type?

I'm not sure if we should duplicate code just for this? Or maybe we should just do something for the HTCondor specific case?

jolange commented 2 years ago

Only for the doc string, actually. Indicating "dict or list" would be ok, I think, but the doc strings for the *Cluster classes are rather different (and rightfully so, I think):

$ ack "job_extra_directives : " dask_jobqueue/*.py -A2
dask_jobqueue/htcondor.py
245:    job_extra_directives : dict
246-        Extra submit file attributes for the job as key-value pairs.
247-        They will be inserted as ``key = value``.

dask_jobqueue/lsf.py
211:    job_extra_directives : list
212-        List of other LSF options, for example -u. Each option will be
213-        prepended with the #LSF prefix.

dask_jobqueue/oar.py
134:    job_extra_directives : list
135-        List of other OAR options, for example `-t besteffort`. Each option will be prepended with the #OAR prefix.
136-

dask_jobqueue/pbs.py
143:    job_extra_directives : list
144-        List of other PBS options. Each option will be prepended with the #PBS prefix.
145-

dask_jobqueue/sge.py
117:    job_extra_directives : list
118-        List of other SGE options, for example -w e. Each option will be
119-        prepended with the #$ prefix.

dask_jobqueue/slurm.py
148:    job_extra_directives : list
149-        List of other Slurm options, for example -j oe. Each option will be prepended with the #SBATCH prefix.
150-

What do you think about making it a parameter of the base class and store it as member variable, but keeping the doc string in the different base classes? Maybe adding a comment to the parameter in the base class where the doc string(s) can be found?

guillaumeeb commented 2 years ago

Sounds good. We should put a generic doc string in the base class, and some more complete description in each cluster implementation.

jolange commented 2 years ago

Ok, I pushed a proposal, now.

guillaumeeb commented 2 years ago

Thanks a lot @jolange for sticking with that!!