Add Configuration #6
Conversation
This adds a basic config file and uses that configuration throughout the dask-yarn codebase. This also switches the convention away from providing memory size in megabytes and instead using strings with units. We provide informative error messages to guide users to correct behavior.
MANIFEST.in
Outdated
| recursive-exclude * __pycache__ | ||
| recursive-exclude * *.py[co] | ||
| include versioneer.py | ||
| include dask_yarn/_version.py |
| worker_memory : int, optional | ||
| The ammount of memory in MB to allocate per worker. | ||
| worker_memory : str, optional | ||
| The ammount of memory to allocate per worker like '2GB' or '4096 MiB'. |
There was a problem hiding this comment.
Could we support both strings and int (in MB)?
There was a problem hiding this comment.
We could, though I think it's a strange default. Of course, it could be argued that it's a default choice that's made for us by Yarn. Current convention in Dask is that if the user provides a number then that number corresponds to the number of bytes, not megabytes.
It's pretty obvious today what the user wanted, and we can automatically convert any number less than 1e6 to megabytes if that would be nicer. I somewhat prefer the current convention because it is fully explicit, but I can see how it might be unpleasant to a user familiar with YARN conventions. Happy to make the change.
There was a problem hiding this comment.
It's easier to add later than to remove. Fine as is.
| @@ -1,11 +1,15 @@ | |||
| from setuptools import setup | |||
|
|
|||
| with open('requirements.txt') as f: | |||
| install_requires = f.read().strip().split('\n') | |||
There was a problem hiding this comment.
I personally prefer to keep it all in the setup.py file, but this isn't a strong preference.
There was a problem hiding this comment.
Most projects today seem to keep things in requirements.txt . I'm inclined to stay with that convention.
I also like this personally because it's easy to look up what a package requires quickly in a data file rather than looking through a setup.py file. Although in this case the setup.py is simple enough that this isn't a very solid argument. The "common convention" one though I think does stand.
| msg = ("You must provide a path to a redeployable environment for the " | ||
| "workers.\n" | ||
| "This is commonly achieved through conda-pack.\n\n" | ||
| "See https://dask-yarn.readthedocs.org/en/latest/environments.html " |
There was a problem hiding this comment.
Not yet! I was going to play with documentation next
This adds a basic config file and uses that configuration throughout the
dask-yarn codebase.
This also switches the convention away from providing memory size in
megabytes and instead using strings with units. We provide informative
error messages to guide users to correct behavior.
cc @jcrist for review. In particular I want to check in and make sure that you're ok with the memory handling conventions. They're different from skein. We can walk back from this a bit if you like.