mila-iqia / milatools

Tools to connect to and interact with the Mila cluster
MIT License
62 stars 12 forks source link

Add `ComputeNode`, `LocalV2` + deprecate `RemoteV1` and `LocalV1` #112

Closed lebrice closed 5 months ago

lebrice commented 6 months ago

What this does

This PR adds some new functions and classes which will be used in the near future to significantly refactor most commands of milatools.

Once these commands are made to use the content of this PR, they will:

Highlights for reviewers (take a look at this first):

I would appreciate it if you could take a quick look at a few particular things, if you feel like it of course:

Here's a little class diagram illustrating the relationship between Runner, LocalV2, RemoteV2, and ComputeNode:

classDiagram
direction BT
class Runner
<<abstract>> Runner
class Runner{
    str hostname
    run(command) CompletedProcess
    run_async(command) CompletedProcess
    get_output(command) str
    get_output_async(command) str
}

LocalV2 --|> Runner : implements

class RemoteV2 {
    LocalV2 local_runner
    async connect(hostname) RemoteV2
}
RemoteV2 --|> Runner : implements

class ComputeNode {
    RemoteV2 login_node
    int job_id
    close()
}
ComputeNode --|> Runner : implements

Refactor:

codecov-commenter commented 6 months ago

Codecov Report

Attention: Patch coverage is 80.25478% with 93 lines in your changes are missing coverage. Please review.

Project coverage is 62.21%. Comparing base (ce86eae) to head (212658b).

Files Patch % Lines
milatools/utils/compute_node.py 67.82% 65 Missing :warning:
milatools/utils/remote_v2.py 88.32% 16 Missing :warning:
milatools/cli/commands.py 41.66% 7 Missing :warning:
milatools/utils/local_v2.py 97.70% 2 Missing :warning:
milatools/utils/runner.py 88.23% 2 Missing :warning:
milatools/cli/utils.py 91.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #112 +/- ## ========================================== - Coverage 65.58% 62.21% -3.37% ========================================== Files 12 15 +3 Lines 1781 2178 +397 ========================================== + Hits 1168 1355 +187 - Misses 613 823 +210 ``` | [Flag](https://app.codecov.io/gh/mila-iqia/milatools/pull/112/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mila-iqia) | Coverage Δ | | |---|---|---| | [integrationtests](https://app.codecov.io/gh/mila-iqia/milatools/pull/112/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mila-iqia) | `62.21% <80.25%> (-3.37%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mila-iqia#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lebrice commented 5 months ago

I will rebase this and make it much cleaner and easier for others to review. I'll mark this as "ready for review" when I'm done. Should probably be done today.