Pass arbitrary kwargs from run_docker/run_k8s to run_native #68

nbren12 · 2020-03-20T20:07:09Z

Changing the signature of run_native (e.g. to add an option changing the logging behavior), requires editing 3 other files: run_kubernetes, run_docker, and the __main__.py. This PR allows run_kubernetes and run_docker to pass arbitrary arguments to run_native provided that they are serializable with json. A side effect is that run_kubernetes can now take an actual configuration dictionary rather than a url pointing to one.

It also tidies up the implementation of run_kubernetes especially. run_docker still has some unavoidable logic to do with creating the bind-mounts, but is slightly cleaner now too.

This routines should not have to change to accomodate a new keyword argument in run_native

mcgibbon

Looks neat! We had a video chat about some small changes and future plans, wrote down my notes in the particular spots they were relevant.

mcgibbon · 2020-03-20T21:51:42Z

fv3config/fv3run/_docker.py

@@ -13,7 +14,7 @@


 def run_docker(
-    config_dict_or_location, outdir, docker_image, runfile=None, keyfile=None
+    config_dict_or_location, outdir, docker_image, runfile=None, keyfile=None, **kwargs


The new arg should be explicit here.

There is no new argument right now. that will be added in the future. I guess I could remove this kwargs for now.

Yes, please remove **kwargs for now.

mcgibbon · 2020-03-20T21:59:06Z

fv3config/fv3run/_docker.py

+    if isinstance(config_dict_or_location, str):
+        with fsspec.open(config_dict_or_location) as f:
+            config_dict = yaml.safe_load(f.read())
+    else:
+        config_dict = config_dict_or_location


Please split this into a helper function, should be enough to solve this function being a little long.

I don't really want to hide this if-statement in a helper function, but I did move the I/O into a helper function called _load_yaml(url).

mcgibbon · 2020-03-20T22:00:40Z

fv3config/fv3run/_docker.py

-        config_location = _get_config_args(
-            config_dict_or_location, config_tempfile, bind_mount_args
-        )


I can't find where config_location is being added to bind_mount_args now, can you point me to it or add it?

It is not longer necessary to save the config dict to a temporary file, it is serialized to json by run_native.command as you can see if you print the contents of python_command.

OK, just be aware that maximum command lengths do exist (though on the order of a hundred KB or so).

mcgibbon · 2020-03-20T22:01:22Z

fv3config/fv3run/_docker.py

@@ -143,6 +123,7 @@ def _get_local_data_bind_mounts(config_dict, bind_mount_args):
 def _get_docker_args(docker_args, bind_mount_args, outdir):
    bind_mount_args += ["-v", f"{os.path.abspath(outdir)}:{DOCKER_OUTDIR}"]
    docker_args += ["--rm", "--user", f"{os.getuid()}:{os.getgid()}"]
+    return DOCKER_OUTDIR


You shouldn't need to return a global constant in a module-private function.

Okay. I did it this way so that DOCKER_OUTDIR would essentially become an implementation detail of the _get_docker_args, but not a big deal to change.

mcgibbon · 2020-03-20T22:02:51Z

fv3config/fv3run/_docker.py


+    outdir_in_docker = _get_docker_args(docker_args, bind_mount_args, outdir)


should just use DOCKER_OUTDIR in place of this local variable (which will currently always be equal to DOCKER_OUTDIR) wherever you would use it.

mcgibbon · 2020-03-20T22:05:04Z

fv3config/fv3run/_kubernetes.py

+    if filesystem._is_local_path(outdir):
+        warnings.warn(
+            f"Output directory {outdir} is a local path, so it will not be accessible "
+            "once the job finishes."
+        )


This warning might not be possible with a command-centric approach, though we could put it instead in the function that generates the command? Or maybe we'll keep this API the same and it will just make use of the new interface. idk.

This warning is specific to k8s, so we wouldn't want it to appear if run_native.command is called for another execution context. I would be fine leaving this since I don't see a problem run_k8s doing some introspection into its arguments. Ultimately, we will probably use run_k8s less as we move to the more general interface for making k8s jobs.

OK, this was mostly a note about an issue you'll run into in the future - you'll need a special command for submitting kubernetes fv3gfs runs basically so that you can post this warning.

mcgibbon · 2020-03-20T22:05:18Z

fv3config/fv3run/_kubernetes.py

@@ -117,12 +119,12 @@ def _submit_job(job, namespace):
    api.create_namespaced_job(body=job, namespace=namespace)


-def _create_job_object(config_location, outdir, docker_image, runfile, kube_config):
+def _create_job_object(command, docker_image, kube_config):


mcgibbon · 2020-03-20T22:05:35Z

fv3config/fv3run/_native.py

+def call_via_subprocess(func):
+    this_module = func.__module__
+
+    def main(argv):
+        import json
+
+        args, kwargs = json.loads(argv[1])
+        func(*args, **kwargs)
+
+    def command(*args, **kwargs) -> str:
+        serialized = json.dumps([args, kwargs])
+        return ["python", "-m", this_module, serialized]
+
+    func.main = main
+    func.command = command
+    return func


mcgibbon · 2020-03-20T22:15:27Z

fv3config/fv3run/_native.py

+        args, kwargs = json.loads(argv[1])
+        func(*args, **kwargs)
+
+    def command(*args, **kwargs) -> str:


can use @functools.wraps to give this function the same call signature as run_native. You may want to figure out how to just modify the call signature without modifying the name or docstring, and see if there are any side-effects I don't know about.

I added the decorator. However, after reading the docs, I don't think functools.wraps does any sort of argument checking. Do you think that feature is important to include, since atm this is not a public API, and we can just assume that run_k8s and run_docker know what they are doing?

It does add argument checking. Looks good.

mcgibbon · 2020-03-20T22:16:25Z

tests/test_fv3run.py

+    @call_via_subprocess
+    def dummy_function(*obj):
+        output.append(hash(obj))
+
+    inputs = (1, 2, 3)
+
+    # first call is via "main"
+    serialized_args = dummy_function.command(*inputs)[-1]
+    argv = [None, serialized_args]
+    dummy_function.main(argv)
+    dummy_ans = output.pop()


If you do add the code that modifies the signature of command to be the same as the wrapped function, be sure to test it :)

I would rather leave this safety guarantee to another PR unless you think it is very important.

It's an easy test. just something like with pytest.raises(TypeError): command(bad=None, args=True). I think it's important enough to include, as it can allow weird, hard-to-track bugs if this regresses in a way that isn't intentional.

nbren12

another "comment"

nbren12 · 2020-03-20T21:25:29Z

tests/test_fv3run.py

@@ -223,43 +217,6 @@ def maybe_get_file(*args, **kwargs):
        pass


-@pytest.mark.parametrize(


This only tested the "fv3config.yml" bind-mount, which is no longer necessary since the configuration dictionary is serialized and passed to run_native directly.

nbren12 · 2020-03-20T21:29:07Z

tests/test_kubernetes.py

@@ -151,13 +153,6 @@ def _check_toleration(toleration):
    assert toleration.value == "climate-sim-pool"


-def _check_command(command, outdir, config_location, runfile):


This is now covered by the run_native tests, since run_kubernetes no longer has primary responsibility for constructing its "command".

OK, as long as it's tested.

nbren12 · 2020-03-21T00:18:42Z

fv3config/fv3run/_docker.py

-        config_location = _get_config_args(
-            config_dict_or_location, config_tempfile, bind_mount_args
-        )


It is not longer necessary to save the config dict to a temporary file, it is serialized to json by run_native.command as you can see if you print the contents of python_command.

nbren12 · 2020-03-23T16:29:25Z

fv3config/fv3run/_docker.py

@@ -13,7 +14,7 @@


 def run_docker(
-    config_dict_or_location, outdir, docker_image, runfile=None, keyfile=None
+    config_dict_or_location, outdir, docker_image, runfile=None, keyfile=None, **kwargs


There is no new argument right now. that will be added in the future. I guess I could remove this kwargs for now.

nbren12 · 2020-03-23T18:51:40Z

fv3config/fv3run/_docker.py

+    if isinstance(config_dict_or_location, str):
+        with fsspec.open(config_dict_or_location) as f:
+            config_dict = yaml.safe_load(f.read())
+    else:
+        config_dict = config_dict_or_location


I don't really want to hide this if-statement in a helper function, but I did move the I/O into a helper function called _load_yaml(url).

nbren12 · 2020-03-23T18:55:46Z

fv3config/fv3run/_kubernetes.py

@@ -5,7 +5,7 @@

 from .. import filesystem
 from .._exceptions import DelayedImportError
-from ._docker import _get_python_command
+from ._native import run_native


nbren12 · 2020-03-23T18:56:24Z

fv3config/fv3run/_kubernetes.py

@@ -26,6 +26,8 @@ def run_kubernetes(
    gcp_secret=None,
    image_pull_policy="IfNotPresent",
    job_labels=None,
+    submit=True,
+    **kwargs,


I will remove the kwargs, since this PR doesn't actually add any new ones yet. The stderr/out handling was gonna be a new PR.

nbren12 · 2020-03-23T18:59:45Z

fv3config/fv3run/_kubernetes.py

+    if filesystem._is_local_path(outdir):
+        warnings.warn(
+            f"Output directory {outdir} is a local path, so it will not be accessible "
+            "once the job finishes."
+        )


This warning is specific to k8s, so we wouldn't want it to appear if run_native.command is called for another execution context. I would be fine leaving this since I don't see a problem run_k8s doing some introspection into its arguments. Ultimately, we will probably use run_k8s less as we move to the more general interface for making k8s jobs.

nbren12 · 2020-03-23T19:06:04Z

fv3config/fv3run/_native.py

+        args, kwargs = json.loads(argv[1])
+        func(*args, **kwargs)
+
+    def command(*args, **kwargs) -> str:


I added the decorator. However, after reading the docs, I don't think functools.wraps does any sort of argument checking. Do you think that feature is important to include, since atm this is not a public API, and we can just assume that run_k8s and run_docker know what they are doing?

nbren12 · 2020-03-23T19:07:25Z

tests/test_fv3run.py

+    @call_via_subprocess
+    def dummy_function(*obj):
+        output.append(hash(obj))
+
+    inputs = (1, 2, 3)
+
+    # first call is via "main"
+    serialized_args = dummy_function.command(*inputs)[-1]
+    argv = [None, serialized_args]
+    dummy_function.main(argv)
+    dummy_ans = output.pop()


I would rather leave this safety guarantee to another PR unless you think it is very important.

mcgibbon

Two remaining small fixes (the first and last comments).

mcgibbon · 2020-03-23T19:55:12Z

fv3config/fv3run/_native.py

+        args, kwargs = json.loads(argv[1])
+        func(*args, **kwargs)
+
+    def command(*args, **kwargs) -> str:


It does add argument checking. Looks good.

mcgibbon · 2020-03-23T19:55:32Z

fv3config/fv3run/_docker.py

@@ -13,7 +14,7 @@


 def run_docker(
-    config_dict_or_location, outdir, docker_image, runfile=None, keyfile=None
+    config_dict_or_location, outdir, docker_image, runfile=None, keyfile=None, **kwargs


Yes, please remove **kwargs for now.

mcgibbon · 2020-03-23T19:57:28Z

fv3config/fv3run/_docker.py

-        config_location = _get_config_args(
-            config_dict_or_location, config_tempfile, bind_mount_args
-        )


OK, just be aware that maximum command lengths do exist (though on the order of a hundred KB or so).

mcgibbon · 2020-03-23T20:00:56Z

fv3config/fv3run/_kubernetes.py

+    if filesystem._is_local_path(outdir):
+        warnings.warn(
+            f"Output directory {outdir} is a local path, so it will not be accessible "
+            "once the job finishes."
+        )


OK, this was mostly a note about an issue you'll run into in the future - you'll need a special command for submitting kubernetes fv3gfs runs basically so that you can post this warning.

mcgibbon · 2020-03-23T20:01:23Z

tests/test_fv3run.py

@@ -223,43 +217,6 @@ def maybe_get_file(*args, **kwargs):
        pass


-@pytest.mark.parametrize(


mcgibbon · 2020-03-23T20:02:49Z

tests/test_kubernetes.py

@@ -151,13 +153,6 @@ def _check_toleration(toleration):
    assert toleration.value == "climate-sim-pool"


-def _check_command(command, outdir, config_location, runfile):


OK, as long as it's tested.

mcgibbon · 2020-03-23T20:05:15Z

tests/test_fv3run.py

+    @call_via_subprocess
+    def dummy_function(*obj):
+        output.append(hash(obj))
+
+    inputs = (1, 2, 3)
+
+    # first call is via "main"
+    serialized_args = dummy_function.command(*inputs)[-1]
+    argv = [None, serialized_args]
+    dummy_function.main(argv)
+    dummy_ans = output.pop()


It's an easy test. just something like with pytest.raises(TypeError): command(bad=None, args=True). I think it's important enough to include, as it can allow weird, hard-to-track bugs if this regresses in a way that isn't intentional.

nbren12 · 2020-03-24T18:28:00Z

@mcgibbon FYI, functools wraps doesn't enforce that the signatures match:

>>> from functools import wraps
>>> def f(a,b):
...     pass
...
>>> @wraps(f)
... def g(a):
...     pass
...
>>> g(10)

functools doesn't do this

mcgibbon

Looks good! Just one stray line which can be removed, then good to merge.

mcgibbon · 2020-03-24T19:03:34Z

fv3config/fv3run/_native.py

+    signature = inspect.signature(func)
+
+    def main(argv):
+        import json


import not needed here

* Update history.rst with changes from #68 * Update HISTORY.rst Co-Authored-By: Jeremy McGibbon <jeremym@vulcan.com> * Update HISTORY.rst Co-Authored-By: Jeremy McGibbon <jeremym@vulcan.com> Co-authored-by: Jeremy McGibbon <jeremym@vulcan.com>

nbren12 added 6 commits March 20, 2020 19:36

Move the definition of the "command" be part of run_native

ec1b811

Let run_k8s automatically wrap run_native

0a00195

This routines should not have to change to accomodate a new keyword argument in run_native

run_docker working

dee2da5

add kwargs

a09ead7

fix tests and lint

293fc56

functool wraps

6bde7b7

mcgibbon requested changes Mar 20, 2020

View reviewed changes

nbren12 added 2 commits March 23, 2020 19:12

respond to review comments

12744c5

remove kweargs

a5fa175

nbren12 commented Mar 23, 2020

View reviewed changes

delete kwargs

dfc541f

mcgibbon requested changes Mar 23, 2020

View reviewed changes

nbren12 added 2 commits March 24, 2020 18:39

Add xfailing test for signature checking

28aa4db

functools doesn't do this

Raise error if signature is wrong

a6f29bb

mcgibbon approved these changes Mar 24, 2020

View reviewed changes

fv3config/fv3run/_native.py Outdated

signature = inspect.signature(func)

def main(argv):

import json

Copy link

Collaborator

mcgibbon Mar 24, 2020

Choose a reason for hiding this comment

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

import not needed here

remoe json import

2cb14ef

nbren12 merged commit bb1c1fa into master Mar 24, 2020

nbren12 deleted the feature/centralize-run-native branch March 24, 2020 19:56

nbren12 added a commit that referenced this pull request Mar 31, 2020

Update history.rst with changes from #68

c534b1b


		outdir_in_docker = _get_docker_args(docker_args, bind_mount_args, outdir)

		@@ -223,43 +217,6 @@ def maybe_get_file(args, *kwargs):
		pass


		@pytest.mark.parametrize(

		@@ -151,13 +153,6 @@ def _check_toleration(toleration):
		assert toleration.value == "climate-sim-pool"


		def _check_command(command, outdir, config_location, runfile):

Pass arbitrary kwargs from run_docker/run_k8s to run_native #68

Pass arbitrary kwargs from run_docker/run_k8s to run_native #68

Conversation

nbren12 commented Mar 20, 2020 • edited Loading

mcgibbon 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

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

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

nbren12 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcgibbon 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

nbren12 commented Mar 24, 2020

mcgibbon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbren12 commented Mar 20, 2020 •

edited

Loading