allow writing into Data Volume in job submission #9

mtaghiza · 2020-03-25T13:48:11Z

No description provided.

glemson

I don't understand the lines like
426/554: datVols.append({'userVolumeId': vol.get('id')...
I thought data volumes are identified by their name, not by a userVolumeId.
Also, the API expect for datavolumes the attribute 'writable', not 'needsWriteAccess'.
I have attached a version of submitShellCommandJob that I have used to work around not being able to write to data volumes. See attached file:

my_submitshelljob.txt

mtaghiza · 2020-04-03T13:40:23Z

the VolumeContainerModel seems to have both a name and an id. I'm now adding both, but probably you remember better whether name is enforced to be unique at registration time.

…ult outputs types for rdb domain queries.

glemson

still some changes requested.
Sorry for delay in reviewing.

glemson · 2021-03-09T13:55:31Z

py2/SciServer/Jobs.py

@@ -410,7 +410,7 @@ def submitNotebookJob(notebookPath, dockerComputeDomain=None, dockerImageName=No
        datVols = [];
        if dataVolumes is None:


I know this is not part of this PR, but I believe that when no dataVolumes are explicitly provided, whether as [] or as None, we should NOT mount any data volumes.

however, in any case I would NOT make them writable, even though the DV may be writable by default. I would be happy if users are explicit in what thye want us to do, and if by accident (not providing explicit list) they mount all data volumes, at least they should not be writable.

Currently behavior is to mount everything if no volumes are specified though. I think that does make for a much more convenient experience for the user instead having to specify a long list of dictionaries of user and data volumes.

@jonashaase : could you confirm you agree with the changes I just made?

glemson · 2021-03-09T13:58:08Z

py2/SciServer/Jobs.py

@@ -422,12 +422,12 @@ def submitNotebookJob(notebookPath, dockerComputeDomain=None, dockerImageName=No
                    if vol.get('name') == dVol.get('name'):
                        found = True;
                        if dVol.get('needsWriteAccess'):
-                            if dVol.get('needsWriteAccess') is True and 'write' in vol.get('allowedActions'):
+                            if dVol.get('needsWriteAccess') is True:


why is the constraint on "'write' in allowedActions" removed?

it's a bug fix (vol does not have 'allowedActions' field)

glemson · 2021-03-09T13:59:29Z

py2/SciServer/Jobs.py

                                datVols.append({'id': vol.get('id'), 'name': vol.get('name'), 'writable': True});
                            else:
                                datVols.append({'id': vol.get('id'), 'name': vol.get('name'), 'writable': False});
                        else:
-                            if 'write' in vol.get('allowedActions'):
+                            if vol.get('writable') is True:


again here if a user does not ask for write access we should not give it by default, even if they are allowed to write there..
I'd rather err on the side of caution.

fixed that.

glemson · 2021-03-09T14:00:11Z

py2/SciServer/Jobs.py

@@ -538,7 +536,7 @@ def submitShellCommandJob(shellCommand, dockerComputeDomain = None, dockerImageN
        datVols = [];
        if dataVolumes is None:
            for vol in dockerComputeDomain.get('volumes'):
-                if 'write' in vol.get('allowedActions'):
+                if vol.get('writable') is True:


see comments above.

… parameter value (but give data volumes only read permissions)

glemson · 2021-03-09T19:52:14Z

additions were made based on a conversation on slack. Will copy that here:
the issue was a bit with the fact that None means something different form [] which I was bitten by quite a few times. could we make this explicit, for example datavolumes='all' be interpreted that way? that's "nice" thing of python, data datatypes are not predefined so 'all' is valid. Prefer that over difference between None and [].

so @channel any votes for using the word 'all', both for data and user volumes to explicitly indicate all should be mounted? for data volumes this would imply all read-only, for user volumes mounting writable would depend on what rights you have?

amitschang · 2021-03-09T20:11:54Z

py3/SciServer/Jobs.py

-        if userVolumes is None:
+        if userVolumes == 'all':


Should we be concerned with backward compatibility here? There may be some cases where users code is passing None and expecting all volumes. Might be good to give a warning about changing behavior prior to all out removal, or at least print warning that behavior has already changed.

glemson · 2021-03-09T20:17:35Z

Agree, warning should be given for some time. Or maybe not even allow None and throw an exception? We currently seem to have None as default. We could make 'all' the default and a user would have had to explicitly have the value set to None to have a problem with backwards compatibility.

mtaghiza · 2021-03-09T20:26:00Z

Could throw an exception, but new documentation and version number warn users as well. Much more disruptive changes might come in the future (like using classes), so users should not expect backwards compatibility for always.

glemson · 2021-03-09T20:38:43Z

I don't think that is realistic to expect our users to check version labels and documentation. They are not explicitly updating their python install or their sciserver modules, they just create a new container. So it is reasonable to expect them to be surprised if their code does not work anymore. And we should give an explicit indication of the reason why.

…submission.

amitschang · 2021-03-09T20:40:57Z

Yes, this is what I am getting after. There does appear to be some possible sense in a None value in this update (though you could equally force [], so it would be good to have a warning printed when using None

glemson · 2021-03-09T20:46:34Z

ok. so choice is how to treat None if it is used as an explicit value.
Throw exception or give warning that it will be transformed to []?
OR for backwards compatibility turn it into 'all', BUT give a warning that this is deprecated?

amitschang · 2021-03-09T20:50:08Z

@mtaghiza just pushed a change to raise an exception, so none will no longer be valid. It will get the job done (nicer would be as you said, turn it to all and put warning - so anyone passing None won't have to fix things immediately)

glemson · 2021-03-09T20:52:02Z

@mtaghiza would you consider changing the behaviour to turn None to 'all' and raise a warning?

mtaghiza · 2021-03-09T20:55:15Z

@mtaghiza would you consider changing the behaviour to turn None to 'all' and raise a warning?

sure, I did, but not in all methods. Just pushed it.

glemson · 2021-03-09T21:01:00Z

I meant that if a user specifies None explicitly for userVolumes or dataVolumes, the code will not throw an exception, but change that variable to 'all' and raise a warning. currently still throws an exception.

mtaghiza · 2021-03-10T13:48:11Z

I meant that if a user specifies None explicitly for userVolumes or dataVolumes, the code will not throw an exception, but change that variable to 'all' and raise a warning. currently still throws an exception.

In this particular case, we might be overcomplicating things with adding a warning for a while an then sometime in the future switch the code to throw the exception. If we simply throw the exception in the first place, it is not the case that all users will immediately need to spend some time and effort updating their code. In fact, it is quite the contrary: it will only affect those users that have explicitly assigned the value to None, and only if they happen to create a new container and run the same code in it at some undetermined point in the future. More over, the code update is so trivial and explicit that those particular users can change the value from None to 'all' in a few seconds. And in real life, these end users generally disregard warnings whatsoever, and wait until the last second to update their code only when it breaks, which they will have to do anyways.

glemson

wow, this has been hanging around way too long.
And I need this code.
Hope others can also have a quick look still.

glemson · 2021-03-09T19:53:55Z

py2/SciServer/Jobs.py

            for vol in dockerComputeDomain.get('volumes'):
-                datVols.append({'name': vol.get('name')});
+                datVols.append({'id': vol.get('id'), 'name': vol.get('name'), 'writable': False});

        else:
            for dVol in dataVolumes:


it is still possible for the input argument dataVolumes to be None. In that case this code would throw an error I think.
maybe protect against that by setting dataVolumes to [] if the input was None?
Same for userVolumes.

glemson · 2021-12-29T11:19:24Z

py2/SciServer/Jobs.py

            for vol in dockerComputeDomain.get('volumes'):
-                datVols.append({'name': vol.get('name')});
+                datVols.append({'id': vol.get('id'), 'name': vol.get('name'), 'writable': False});


Just noting here that we treat data volumes differently from user volumes. in the latter needsWriteAccess is decided by user privilege when 'all', for former we want users to explicitly ask for write access.

allow writing into Data Volume in job submission

346b9ca

mtaghiza requested a review from glemson March 25, 2020 13:48

mtaghiza assigned glemson Mar 25, 2020

glemson requested changes Mar 25, 2020

View reviewed changes

mtaghiza added 2 commits April 3, 2020 08:39

add name to data volume definition in job submitting methods

59348ba

fixed typo

804b3ce

mtaghiza added 3 commits April 21, 2020 17:36

fixed documentation, open jobs, and allowed actions checks

b869120

added getJobQueues function, and improved way of setting/defining res…

9d0f19a

…ult outputs types for rdb domain queries.

small fix

127fcac

glemson requested changes Mar 9, 2021

View reviewed changes

allow including all data volumes and user volumes by specifying 'all'…

4781501

… parameter value (but give data volumes only read permissions)

glemson requested a review from amitschang March 9, 2021 19:50

amitschang reviewed Mar 9, 2021

View reviewed changes

raise exception with None values for user/data volumes in docker job …

6db2371

…submission.

include 'all' option in missing remaining methods

b2ad78c

fifed 'all' assignement

ace43f8

glemson approved these changes Dec 28, 2021

View reviewed changes

glemson reviewed Dec 29, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow writing into Data Volume in job submission #9

allow writing into Data Volume in job submission #9

mtaghiza commented Mar 25, 2020

glemson left a comment

mtaghiza commented Apr 3, 2020

glemson left a comment

glemson Mar 9, 2021

glemson Mar 9, 2021

jonashaase Mar 9, 2021

mtaghiza Mar 9, 2021

glemson Mar 9, 2021

mtaghiza Mar 9, 2021

glemson Mar 9, 2021

mtaghiza Mar 9, 2021

glemson Mar 9, 2021

glemson commented Mar 9, 2021

amitschang Mar 9, 2021

glemson commented Mar 9, 2021 via email

mtaghiza commented Mar 9, 2021

glemson commented Mar 9, 2021 via email

amitschang commented Mar 9, 2021

glemson commented Mar 9, 2021

amitschang commented Mar 9, 2021

glemson commented Mar 9, 2021

mtaghiza commented Mar 9, 2021

glemson commented Mar 9, 2021

mtaghiza commented Mar 10, 2021 •

edited

Loading

glemson left a comment

glemson Mar 9, 2021

glemson Dec 29, 2021 •

edited

Loading

		@@ -410,7 +410,7 @@ def submitNotebookJob(notebookPath, dockerComputeDomain=None, dockerImageName=No
		datVols = [];
		if dataVolumes is None:

allow writing into Data Volume in job submission #9

Are you sure you want to change the base?

allow writing into Data Volume in job submission #9

Conversation

mtaghiza commented Mar 25, 2020

glemson left a comment

Choose a reason for hiding this comment

mtaghiza commented Apr 3, 2020

glemson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemson commented Mar 9, 2021

Choose a reason for hiding this comment

glemson commented Mar 9, 2021 via email

mtaghiza commented Mar 9, 2021

glemson commented Mar 9, 2021 via email

amitschang commented Mar 9, 2021

glemson commented Mar 9, 2021

amitschang commented Mar 9, 2021

glemson commented Mar 9, 2021

mtaghiza commented Mar 9, 2021

glemson commented Mar 9, 2021

mtaghiza commented Mar 10, 2021 • edited Loading

glemson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemson Dec 29, 2021 • edited Loading

Choose a reason for hiding this comment

mtaghiza commented Mar 10, 2021 •

edited

Loading

glemson Dec 29, 2021 •

edited

Loading