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

LSF: Add unit scaling other than MB #274

Merged
merged 5 commits into from
Jun 8, 2019
Merged

Conversation

Lnaden
Copy link
Contributor

@Lnaden Lnaden commented May 23, 2019

The lsf.conf file can specify units other than MB for memory, and
any other large units of scaling, through the LSF_UNIT_FOR_LIMITS
variable.

Dask Jobqueue assumes that the units are MB, which is true under
the default setup of LSF, but not always.

  • If the admin has changed it to something else in the file.
  • If the file does not exist, LSF assumes KB.

This PR adds the ability for users to both specify or allow auto
detection of the unit system through an lsf_units kwarg or lsf-units
YAML arg.

Auto-detection routine is based on the LSF 9.1.3 docs for where to
look, defaults, and variable names.

The `lsf.conf` file can specify units other than MB for memory, and
any other large units of scaling, through the `LSF_UNIT_FOR_LIMITS`
variable.

Dask Jobqueue assumes that the units are MB, which is true under
the default setup of LSF, but not always.

* If the admin has changed it to something else in the file.
* If the file does not exist, LSF assumes KB.

This PR adds the ability for users to both specify or allow auto
detection of the unit system through an `lsf_units` kwarg or `lsf-units`
YAML arg.

Auto-detection routine is based on the LSF 9.1.3 docs for where to
look, defaults, and variable names.
@Lnaden
Copy link
Contributor Author

Lnaden commented May 23, 2019

For reference, here are the official LSF docs on the mater

https://www.ibm.com/support/knowledgecenter/en/SSETD4_9.1.3/lsf_config_ref/lsf.conf.lsf_unit_for_limits.5.html

@Lnaden
Copy link
Contributor Author

Lnaden commented May 23, 2019

I think I fixed all the bugs now, I'm not sure where I introduced trailing whitespace though, I don't think I did unless the number of blank lines in the docstrings/YAML file is important?

@guillaumeeb
Copy link
Member

Thanks @Lnaden for this, I did not have the time to take a look at it tight now, but I'll try as soon as possible. In the mean time if any other maintainer or experimented user can look at this, this could be nice!

Copy link
Member

@guillaumeeb guillaumeeb left a comment

Choose a reason for hiding this comment

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

Okay thanks for the work here !

This looks really good, the only thing that is missing is actual tests. Could you provide a test with some given unit, and a test for lsf_detect_units, which is the most complicated part?

@guillaumeeb
Copy link
Member

I don't think I did unless the number of blank lines in the docstrings/YAML file is important?

You've got a trailing white space on line 35 of lsf.py, in the docstring.

Also did you apply black on your codes?

@Lnaden
Copy link
Contributor Author

Lnaden commented May 27, 2019

I'll work on a test and black today. There are also some changes in the 10.1+ version of LSF, so I'm going to make changes to work in both

Add test for the auto-unit detection
Black ran
Changed detection to include LSF 10.1+ versions as well
@Lnaden
Copy link
Contributor Author

Lnaden commented May 27, 2019

I made some changes to include the LSF 10.1+ way of doing things, you can apparently specify everything from KB up to ZB, and even then in single letter short-hand. I think there may be an issue with the ceil function if an LSF cluster has ZB or something else high order specified, but I can't confirm that so may have to leave that as an untested corner case?

For reference, here is the 10.1 docs:

https://www.ibm.com/support/knowledgecenter/en/SSWRJV_10.1.0/lsf_config_ref/lsf.conf.lsf_unit_for_limits.5.html

I made the following changes as well as requested:

  • Fixed trailing whitespace
  • Added tests for the LSF auto-unit detection routine
  • Ran Black

Copy link
Member

@guillaumeeb guillaumeeb left a comment

Choose a reason for hiding this comment

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

Thanks! looks good!

@guillaumeeb guillaumeeb merged commit 35f284d into dask:master Jun 8, 2019
@Lnaden Lnaden deleted the lsf_units branch June 13, 2019 16:56
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.

2 participants