Add type annotations and more tests #61

lebrice · 2023-10-06T20:14:43Z

No description provided.

satyaog

Note: I'm feeling less and less comfortable with the master branch being deeply modified (with the new _* default funcs). It doesn't change anything for the end user and prevents us from quickly bringing a fix to users or simply fix a dependency version. But we're too far in the changes now

satyaog · 2023-10-17T21:37:04Z

milatools/cli/local.py

+        _check_output_fn: Callable[
+            Concatenate[str | Sequence[str], P], str
+        ] = subprocess.check_output,


This is to allow mocking right? This doesn't look very clean to me but I see why this could be time consuming to find a solution so maybe we could add a # TODO: This is to ease mocking in test. Find a cleaner way that doesn't involves a _check_output_fn exposed to the user

It seams it's not used in mocks so I would remove this from the exposed arguments list unless I'm not seeing something?

We can mock subprocess.check_output directly.

The reason for this is mainly for type-checking purposes. We don't want to allow passing invalid **kwargs down to subprocess.check_output. This indicates to the type checker that this method has the same signature as subprocess.check_output (the function it is wrapping), and only accepts these arguments.

The alternative would be to remove the **kwargs and explicitly duplicate all the arguments of subprocess.check_output (and their default values) that in all the places where we use this method.

Removed the ParamSpecs and made the signatures explicit in 1a6ecd5

There's no other way? Type checking requires adding a function's argument? Would something like this be possible?

return subprocess.check_output: Callable[ Concatenate[str | Sequence[str], P], str ](cmd, *args, **kwargs)

I'm sorry I don't quite understand the question, and no that's not possible. Perhaps we could chat about this in person?

satyaog · 2023-10-17T22:06:44Z

milatools/cli/local.py

-                raise
+            raise e


raise e changes the stacktrace, in opposition of raise. Probably better to keep only raise

Fixed in dcb067c

satyaog · 2023-10-19T21:59:02Z

milatools/cli/profile.py

+class Choice(qn.Choice, Generic[_T]):
+    value: _T
+
+    def __init__(
+        self,
+        title: FormattedText,
+        value: _T | None = None,
+        disabled: str | None = None,
+        checked: bool | None = False,
+        shortcut_key: str | bool | None = True,
+    ) -> None:
+        super().__init__(
+            title=title,
+            value=value,
+            disabled=disabled,
+            checked=checked,
+            shortcut_key=shortcut_key,
+        )


Is this also to help mocking in testings?

No, this just improves the typing of tests by letting the editor know that qn.Choice is a generic type whose value needs to match it's constructor argument.

I'll add a doc here to explain this

Added more explanation in 8e92d4c

Ah it's a bit unfortunate to add a layer only for typing but if it helps. Do you think in the future we should put all typing helpers in a separate file?

milatools/cli/remote.py

satyaog · 2023-10-20T13:24:43Z

milatools/cli/remote.py

+    @overload
+    def run(
+        self,
+        cmd: str,
+        display: bool | None = None,
+        hide: bool = False,
+        warn: bool = False,
+        asynchronous: bool = False,
+        out_stream: TextIO | None = None,
+        **kwargs,
+    ) -> invoke.runners.Result | invoke.runners.Promise:
+        ...


What is the reason behind having these @overload methods?

This is to help narrow down the return types of the run method. When you call it with asynchronous=True, you get a Promise, and when you call it with asynchronous=False, you get a Result. When asynchronous is a bool with unknown value, the result may be either a Promise or Result.

satyaog · 2023-10-20T18:26:15Z

tests/cli/test_remote.py

+    @dont_run_for_real
+    @pytest.mark.parametrize("persist", [True, False])
+    def test_persist(self, mock_connection: Connection, persist: bool):
+        alloc = ["--time=00:01:00"]
+        transforms = [some_transform]
+        remote = SlurmRemote(
+            mock_connection, alloc=alloc, transforms=transforms, persist=persist
+        )
+        persisted = remote.persist()
+
+        # NOTE: Feels dumb to do this. Not sure what I should be doing otherwise.
+        assert persisted.connection == remote.connection
+        assert persisted.alloc == remote.alloc
+        assert persisted.transforms == [
+            some_transform,
+            persisted.srun_transform_persist,
+        ]
+        assert persisted._persist is True


Same, I think we could mock and use a FileRegressionFixture or something like that. And maybe have a test with a real connection to a slurm cluster

satyaog · 2023-10-20T18:36:49Z

tests/cli/test_remote.py

+    @dont_run_for_real
+    @pytest.mark.skip(reason="Seems a bit hard to test for what it's worth..")
+    def test_srun_transform_persist(self, mock_connection: Connection):
+        alloc = ["--time=00:01:00"]
+        transforms = [some_transform]
+        persist: bool = False
+        remote = SlurmRemote(
+            mock_connection, alloc=alloc, transforms=transforms, persist=persist
+        )
+        output_file = "<some_file>"
+        assert (
+            remote.srun_transform_persist("bob")
+            == f"bob; touch {output_file}; tail -n +1 -f {output_file}"
+        )


Same, I think we could mock and use a FileRegressionFixture or something like that

Yeah, I think I see what you mean, you want to avoid the hard-coded portion of the test and move that to a regression file instead?

Yes that's what I had in mind

satyaog · 2023-10-20T18:38:06Z

tests/cli/test_remote.py

+    @dont_run_for_real
+    @disable_internet_access
+    def test_ensure_allocation_persist(self, mock_connection: Connection):
+        alloc = ["--time=00:01:00"]
+        transforms = [some_transform]
+        remote = SlurmRemote(
+            mock_connection, alloc=alloc, transforms=transforms, persist=True
+        )
+
+        # TODO: Not sure if this test has any use at this point..
+        remote.extract = Mock(
+            spec=remote.extract,
+            spec_set=True,
+            return_value=(
+                Mock(spec=invoke.runners.Runner, spec_set=True),
+                {"node_name": "bob", "jobid": "1234"},
+            ),
+        )
+
+        results, runner = remote.ensure_allocation()
+
+        remote.extract.assert_called_once_with(
+            "echo @@@ $(hostname) @@@ && sleep 1000d",
+            patterns={
+                "node_name": "@@@ ([^ ]+) @@@",
+                "jobid": "Submitted batch job ([0-9]+)",
+            },
+            hide=True,
+        )
+        assert results == {"node_name": "bob", "jobid": "1234"}
+        # raise NotImplementedError("TODO: Imporant and potentially complicated test")


I think we can only test this with a real connection to a slurm cluster

Agreed, this kind of tests doesn't feel smart.

satyaog · 2023-10-20T18:38:58Z

tests/cli/test_remote.py

+    @dont_run_for_real
+    @disable_internet_access
+    def test_ensure_allocation_without_persist(self, mock_connection: Connection):
+        alloc = ["--time=00:01:00"]
+        transforms = [some_transform]
+        remote = SlurmRemote(
+            mock_connection, alloc=alloc, transforms=transforms, persist=False
+        )
+
+        def write_stuff(
+            command: str,
+            asynchronous: bool,
+            hide: bool,
+            warn: bool,
+            pty: bool,
+            out_stream: QueueIO,
+        ):
+            assert command == f"bash -c 'salloc {shjoin(alloc)}'"
+            out_stream.write("salloc: Nodes bob-123 are ready for job")
+            return unittest.mock.DEFAULT
+
+        mock_connection.run.side_effect = write_stuff
+        results, runner = remote.ensure_allocation()
+
+        mock_connection.run.assert_called_once_with(
+            f"bash -c 'salloc {shjoin(alloc)}'",
+            hide=False,
+            asynchronous=True,
+            out_stream=unittest.mock.ANY,
+            pty=True,
+            warn=False,
+        )
+        assert results == {"node_name": "bob-123"}
+        # raise NotImplementedError("TODO: Imporant and potentially complicated test")


Same, I think we could mock and use a FileRegressionFixture or something like that

I don't see how that would work, perhaps we can chat about this in person

satyaog · 2023-10-20T18:40:41Z

milatools/cli/local.py

+    def run(
+        self,
+        cmd: Sequence[str],
+        _run_fn: Callable[Args[P], CompletedProcess[str]] = subprocess.run,
+        *args: P.args,
+        **kwargs: P.kwargs,
+    ) -> CompletedProcess[str]:


*args should come before _run_fn otherwise run([cmd ...], "arg1", "arg2", {"kwarg3":"kwarg3"}) will result in

cmd = [cmd...] _run_fn = "arg1" *args = ("arg2",) **kwarg3 = {"kwarg3":"kwarg3"}

Suggested change

def run(

self,

cmd: Sequence[str],

_run_fn: Callable[Args[P], CompletedProcess[str]] = subprocess.run,

*args: P.args,

**kwargs: P.kwargs,

) -> CompletedProcess[str]:

def run(

self,

cmd: Sequence[str],

*args: P.args,

_run_fn: Callable[Args[P], CompletedProcess[str]] = subprocess.run,

**kwargs: P.kwargs,

) -> CompletedProcess[str]:

I'd just remove *args altogether if the cmd is already a list. All other arguments should be passed by keyword.

I'll fix the ordering of arguments, but the explanation for the *args and **kwargs and paramspec is the same as in https://github.com/mila-iqia/milatools/pull/61/files#r1362819307

A few notes:

The wrapped function needs to appear before the *args and **kwargs for us to be able to mimic it's signature with ParamSpec for our *args or **kwargs.

You'd get a type-checking error if you tried to do run([cmd ...], "arg1", "arg2", {"kwarg3":"kwarg3"}) . You're right though, it wouldn't work if one tried to use this function when passing positional arguments that make sense for subprocess.run.

Assuming that we agree that we want these kinds of methods to be properly typed, we have two options as far as I know:

Keeping this ParamSpec as-is

List the arguments of the wrapped function explicitly

The second option would duplicate some of the code and the types of the signature of the wrapped function, but might be more explicit.

Let me know what you think.

Ok I opted for the second option in this case, fixed this in 1a6ecd5

.pre-commit-config.yaml

breuleux · 2023-10-31T19:50:34Z

milatools/cli/local.py

+        _check_output_fn: Callable[
+            Concatenate[str | Sequence[str], P], str
+        ] = subprocess.check_output,


We can mock subprocess.check_output directly.

breuleux · 2023-10-31T19:53:02Z

milatools/cli/local.py

+    def run(
+        self,
+        cmd: Sequence[str],
+        _run_fn: Callable[Args[P], CompletedProcess[str]] = subprocess.run,
+        *args: P.args,
+        **kwargs: P.kwargs,
+    ) -> CompletedProcess[str]:


I'd just remove *args altogether if the cmd is already a list. All other arguments should be passed by keyword.

lebrice · 2023-11-01T17:00:12Z

Note: I'm feeling less and less comfortable with the master branch being deeply modified (with the new _* default funcs). It doesn't change anything for the end user and prevents us from quickly bringing a fix to users or simply fix a dependency version. But we're too far in the changes now

Hey @satyaog, @breuleux thanks for the reviews, sorry I didn't reply earlier.

I just want to emphasize something: This PR doesn't change anything about the behaviour of the code. The type hints that are added describe the types and signatures of the methods as they are currently.

There's a very slight exception with these these _* default functions, which are there for typing purposes and don't have any effect on the runtime. I'm okay with removing them and making the signatures explicit, but this would represent a much bigger change in the implementation of these methods than to just add type annotations to the *args and **kwargs and adding these unused keyword parameters for the wrapped functions.

As for the unit tests, I guess having meh integration tests would be much better than having no tests at all. I think making changes to the master branch to introduce tests actually really aligns with your point: We don't want breaking changes to join master. The problem is, until we add some tests, we have no idea what's breaking or not. We need to take a first jump at some point.

satyaog

I like most of it although I'm still perplex with some of the extra explicit arguments listed to only mirror the typing of underlying functions, typing helper classes and overloads. When there's no friction I really like typing but when it looks like a tedious detour I'm less a fan.

I'm approving but the only thing I'd like to discuss before merge is test_remote.py and see if we could simplify the thing by only looking at the command that are going to be executed for most test, which can be done offline.

satyaog · 2023-11-10T17:12:34Z

milatools/cli/local.py

+        _check_output_fn: Callable[
+            Concatenate[str | Sequence[str], P], str
+        ] = subprocess.check_output,


There's no other way? Type checking requires adding a function's argument? Would something like this be possible?

return subprocess.check_output: Callable[ Concatenate[str | Sequence[str], P], str ](cmd, *args, **kwargs)

satyaog · 2023-11-10T18:01:27Z

milatools/cli/profile.py

+        entry: str = qn.autocomplete(
+            "",
+            choices=list(modchoices.keys()),
+            style=qn.Style([("answer", "fg:default bg:default")]),
+        ).unsafe_ask()
+        entry = entry.strip()


maybe?

Suggested change

entry: str = qn.autocomplete(

"",

choices=list(modchoices.keys()),

style=qn.Style([("answer", "fg:default bg:default")]),

).unsafe_ask()

entry = entry.strip()

entry: str = qn.autocomplete(

"",

choices=list(modchoices.keys()),

style=qn.Style([("answer", "fg:default bg:default")]),

).unsafe_ask().strip()

satyaog · 2023-11-10T18:08:06Z

milatools/cli/profile.py

    if env == "<OTHER>":
-        env = askpath("Enter the path to the environment to use.", remote)
+        return askpath("Enter the path to the environment to use.", remote)

-    elif env == "<CREATE>":
-        pyver = qn.select(
+    if env == "<CREATE>":


I personally find cleaner a single return point per function but that is only personally

satyaog · 2023-11-10T18:13:16Z

milatools/cli/profile.py

+class Choice(qn.Choice, Generic[_T]):
+    value: _T
+
+    def __init__(
+        self,
+        title: FormattedText,
+        value: _T | None = None,
+        disabled: str | None = None,
+        checked: bool | None = False,
+        shortcut_key: str | bool | None = True,
+    ) -> None:
+        super().__init__(
+            title=title,
+            value=value,
+            disabled=disabled,
+            checked=checked,
+            shortcut_key=shortcut_key,
+        )


Ah it's a bit unfortunate to add a layer only for typing but if it helps. Do you think in the future we should put all typing helpers in a separate file?

satyaog · 2023-11-10T19:34:37Z

tests/cli/test_remote.py

+    @dont_run_for_real
+    @pytest.mark.skip(reason="Seems a bit hard to test for what it's worth..")
+    def test_srun_transform_persist(self, mock_connection: Connection):
+        alloc = ["--time=00:01:00"]
+        transforms = [some_transform]
+        persist: bool = False
+        remote = SlurmRemote(
+            mock_connection, alloc=alloc, transforms=transforms, persist=persist
+        )
+        output_file = "<some_file>"
+        assert (
+            remote.srun_transform_persist("bob")
+            == f"bob; touch {output_file}; tail -n +1 -f {output_file}"
+        )


Yes that's what I had in mind

satyaog · 2023-11-10T19:37:37Z

tests/cli/test_remote.py

+    @dont_run_for_real
+    @pytest.mark.parametrize("persist", [True, False, None])
+    def test_with_transforms(self, mock_connection: Connection, persist: bool | None):
+        alloc = ["--time=00:01:00"]
+        transforms = [some_transform]
+        original_persist: bool = False
+        remote = SlurmRemote(
+            mock_connection,
+            alloc=alloc,
+            transforms=transforms,
+            persist=original_persist,
+        )
+        new_transforms = [some_other_transform]
+        transformed = remote.with_transforms(*new_transforms, persist=persist)
+        # NOTE: Feels dumb to do this. Not sure what I should be doing otherwise.
+        assert transformed.connection == remote.connection
+        assert transformed.alloc == remote.alloc
+        assert transformed.transforms == [
+            some_transform,
+            some_other_transform,
+            (
+                transformed.srun_transform_persist
+                if persist
+                else transformed.srun_transform
+            ),
+        ]
+        assert transformed._persist == (remote._persist if persist is None else persist)


Yes we can do that. I don't think we actually need to run the command, moreover if we don't check the output. We could only save the command string in a FileRegressionFixture. Then we could potentially wrap a couple of test in a single one with parameters

Signed-off-by: Fabrice Normandin <[email protected]>

Co-authored-by: satyaog <[email protected]>

Signed-off-by: Fabrice Normandin <[email protected]>

lebrice · 2023-11-13T21:29:12Z

Closing in favour of #76 and #75 (as well as a lot of overlapping changes with the now-merged #74 )

lebrice changed the title ~~Add type annotations and unit tests~~ Add type annotations and more unit tests Oct 6, 2023

lebrice changed the title ~~Add type annotations and more unit tests~~ Add type annotations and more tests Oct 6, 2023

lebrice force-pushed the tests branch from f1a5a73 to 98e5e40 Compare October 10, 2023 18:04

lebrice mentioned this pull request Oct 11, 2023

[MT-67] No ssh connection to get the $HOME path #55

Closed

lebrice marked this pull request as ready for review October 11, 2023 18:44

satyaog requested changes Oct 20, 2023

View reviewed changes

breuleux reviewed Oct 31, 2023

View reviewed changes

lebrice requested review from satyaog and breuleux November 9, 2023 20:22

satyaog approved these changes Nov 10, 2023

View reviewed changes

lebrice and others added 19 commits November 13, 2023 11:51

Add type annotations to remote.py

c480932

Signed-off-by: Fabrice Normandin <[email protected]>

Add type annotations and tweak local.py slightly

0e18cee

Signed-off-by: Fabrice Normandin <[email protected]>

Add type hints to profile.py

abb2dfd

Signed-off-by: Fabrice Normandin <[email protected]>

Experiment with tests for fabric code

2e7a787

Signed-off-by: Fabrice Normandin <[email protected]>

Comment out test while debugging fabric fixtures

1e51ef3

Signed-off-by: Fabrice Normandin <[email protected]>

Add type hints and slightly tweak remote.py

5e48a4f

Signed-off-by: Fabrice Normandin <[email protected]>

Start adding tests for the Remote class

e839c0e

Signed-off-by: Fabrice Normandin <[email protected]>

Continue adding more tests for Remote class

499eb2b

Signed-off-by: Fabrice Normandin <[email protected]>

(wip) Add back stashed changes

63200d9

Signed-off-by: Fabrice Normandin <[email protected]>

Deprecate the Remote.get_lines method

ee1371b

Signed-off-by: Fabrice Normandin <[email protected]>

Add more tests for Remote and a new flag

bc1e0f2

Signed-off-by: Fabrice Normandin <[email protected]>

Don't raise a warning for get_lines at runtime

1e63239

Signed-off-by: Fabrice Normandin <[email protected]>

(temp) working on test_extract, hard to test

257428a

Signed-off-by: Fabrice Normandin <[email protected]>

Add missing tests, deprecate unused Remote methods

dc33526

Signed-off-by: Fabrice Normandin <[email protected]>

Add some tests for SlurmRemote

9517ec3

Signed-off-by: Fabrice Normandin <[email protected]>

Fix some of the tests, work on ensure_allocation

c3b0838

Signed-off-by: Fabrice Normandin <[email protected]>

Fix issue with typing.Literal import

2013f05

Signed-off-by: Fabrice Normandin <[email protected]>

Add fabric[pytest] as a dev dependency

693f606

Signed-off-by: Fabrice Normandin <[email protected]>

Allow more tests to run "for real"

d1a0ed2

Signed-off-by: Fabrice Normandin <[email protected]>

lebrice and others added 18 commits November 13, 2023 11:53

Fix broken tests

e146a26

Signed-off-by: Fabrice Normandin <[email protected]>

Add a "proper" mark and config option for internet

6fe8818

Signed-off-by: Fabrice Normandin <[email protected]>

Fix typing.Literal import for py<3.9

416ac4c

Signed-off-by: Fabrice Normandin <[email protected]>

Fix isort

e04446e

Signed-off-by: Fabrice Normandin <[email protected]>

Add a .pre-commit.yaml file that fixes CI stuff

37f4168

Signed-off-by: Fabrice Normandin <[email protected]>

Fix the test installs and update poetry lockfile

1bd7966

Signed-off-by: Fabrice Normandin <[email protected]>

Add missing pytest-mock dev dependency

6e7d233

Signed-off-by: Fabrice Normandin <[email protected]>

Install dev dependencies in build.yml workflow

9ad989e

Signed-off-by: Fabrice Normandin <[email protected]>

Fix TypedDict import issue with python3.7

b7c1818

Signed-off-by: Fabrice Normandin <[email protected]>

Remove use of shlex.join for py3.7

dc01985

Signed-off-by: Fabrice Normandin <[email protected]>

Apply suggestions from code review

d6073de

Co-authored-by: satyaog <[email protected]>

Add .vscode to .gitignore

bae90ad

Signed-off-by: Fabrice Normandin <[email protected]>

Remove Jupyter notebook pre-commit hook

d40005d

Signed-off-by: Fabrice Normandin <[email protected]>

Better stacktrace when raising a FileNotFoundError

9cbe10a

Signed-off-by: Fabrice Normandin <[email protected]>

Remove the ParamSpecs, make methods sigs explicit

9ac157e

Signed-off-by: Fabrice Normandin <[email protected]>

Add comments and clarify types in profile.py

e64ea35

Signed-off-by: Fabrice Normandin <[email protected]>

Fix expected call in unit test

938fc59

Signed-off-by: Fabrice Normandin <[email protected]>

Fix isort issue

b74e42b

Signed-off-by: Fabrice Normandin <[email protected]>

lebrice force-pushed the tests branch from 3d3f3f0 to b74e42b Compare November 13, 2023 16:53

lebrice added 5 commits November 13, 2023 12:28

Add missing type signatures in tests/cli/common.py

576910b

Signed-off-by: Fabrice Normandin <[email protected]>

Add pytest-socket test dependency

615e6d8

Signed-off-by: Fabrice Normandin <[email protected]>

Make the --enable-internet arg work

2ab3994

Signed-off-by: Fabrice Normandin <[email protected]>

Fix change in tests due to Local.run arg change

992197d

Signed-off-by: Fabrice Normandin <[email protected]>

Fix small typing errors (import location, Self)

e1f34e5

Signed-off-by: Fabrice Normandin <[email protected]>

This was referenced Nov 13, 2023

(only) add type hints #75

Merged

(only) add more unit tests #76

Merged

lebrice closed this Nov 13, 2023

Add type annotations and more tests #61

Add type annotations and more tests #61

Conversation

lebrice commented Oct 6, 2023

satyaog 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

lebrice Nov 13, 2023 • edited Loading

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

lebrice Nov 1, 2023 • edited Loading

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

lebrice Nov 6, 2023 • edited Loading

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

lebrice commented Nov 1, 2023 • edited Loading

satyaog 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

lebrice commented Nov 13, 2023

lebrice Nov 13, 2023 •

edited

Loading

lebrice Nov 1, 2023 •

edited

Loading

lebrice Nov 6, 2023 •

edited

Loading

lebrice commented Nov 1, 2023 •

edited

Loading