Repository navigation
Conversation
amitschang
left a comment
There was a problem hiding this comment.
This makes me notice that the setup file lacks any dependencies, maybe this should be done as a separate issue, or should the requirements for this be added to setup.py (like:
install_requires = ['requests', 'dask', 'dask[distributed]']?
| """ | ||
| Creates a new client for the specified Dask cluster. | ||
|
|
||
| If `compute_url` and `ref_id` are not set, cluster connection properties will be read from the ``~/dask-cluster.json`` file injected into new SciServer Compute containers automatically when they are created with an attached Dask cluster. |
| with open(base_dir + '/ca.pem', 'w+b') as f: | ||
| f.write(b64decode(data['ca'])) |
There was a problem hiding this comment.
I'm a bit confused by the combination of PEM file with the binary file option and base64 decoding. The PEM is a plain ASCII file with the certificate/key contents base64 encoded.
There was a problem hiding this comment.
That's because Compute encodes the PEM files in base64 before putting them in JSON fields. Maybe not strictly necessary, but we get a nice long string with no line breaks, always nice to wrap files in base64, even if they are supposed to be ascii... Similarly, for example, in k8s PEM files would be encoded as base64 if they were put in a secret.
There was a problem hiding this comment.
ok, this makes sense. The +b would still cause some confusion though, I think
There was a problem hiding this comment.
Why? b64decode returns bytes.
There was a problem hiding this comment.
but those bytes ought to be ASCII, in any case, I'm sure it would not make a material difference
There was a problem hiding this comment.
I don't quite understand... Simply removing b would result in an error, obviously. To make it work in text mode I would have to do something like this (unless I'm missing something obvious):
with open(base_dir + '/ca.pem', 'w+',encoding='ascii') as f:
f.write(b64decode(data['ca']).decode('ascii'))
This looks more, not less confusing to me... Wouldn't working with bytes be more generic and less error prone?
There was a problem hiding this comment.
I guess my point is that since that we expect that to be ascii data and not binary, that doing
with open(base_dir + '/ca.pem', 'w') as f:
f.write(b64decode(data['ca']).decode('ascii'))Is a bit more explicit, and would fail fast if the data were in fact not encoded as such - rather than down the line when dask workers are making a connection.
of course it is fine as-is, so I already hit the approval button :)
|
Regarding the dependencies, I'd rather create a separate issue, because it's not just a Dask problem. |
glemson
left a comment
There was a problem hiding this comment.
I see no problems (which does not mean there are none).
|
Perhaps |
Yes, this would make good sense (if so, we should not forget to make a helm change for computedomain chart to get that set properly in k8s installs) |
|
@amitschang Ok, I've made that change. Feel free to update k8s charts. |
|
Is the user going to manually copy the ref_id parameter if the code is not run inside sciserver-compute? |
|
@mtaghiza ref_id of available Dask clusters are shown on the Dask tab in the Compute UI, so, at least for now, the idea is to copy the ids manually from there. There is also a Compute API method that will get you a list of all available clusters, but it might need some more work so I'm not including it in this SciScript update yet. In most cases users won't need to provide ref_id manually though. |
|
Might need to update the library version value (unfortunately hardcoded in the files) before merging to master |
Adding the new Dask module. It currently has only one function that creates a new Dask client.
Usage: