search

mila-iqia / milatools

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

Add `mila sync code-extensions` command, improve mila code extensions sync with DRAC clusters [MT-79] #103

Closed lebrice closed 7 months ago

lebrice commented 8 months ago

preview:

https://github.com/mila-iqia/milatools/assets/13387299/1f56da91-faa8-42b1-a4c6-53553d68171d

codecov-commenter commented 8 months ago

Codecov Report

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

Project coverage is 59.20%. Comparing base (7c733d1) to head (2daba0b).

Files Patch % Lines
milatools/utils/vscode_utils.py 28.02% 131 Missing :warning:
milatools/utils/remote_v2.py 75.00% 29 Missing :warning:
milatools/cli/commands.py 43.33% 17 Missing :warning:
milatools/utils/parallel_progress.py 90.76% 6 Missing :warning:
milatools/cli/utils.py 80.00% 4 Missing :warning:
milatools/cli/remote.py 90.47% 2 Missing :warning:
milatools/cli/profile.py 75.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #103 +/- ## ========================================== - Coverage 61.73% 59.20% -2.53% ========================================== Files 9 12 +3 Lines 1445 1777 +332 ========================================== + Hits 892 1052 +160 - Misses 553 725 +172 ``` | [Flag](https://app.codecov.io/gh/mila-iqia/milatools/pull/103/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/103/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mila-iqia) | `?` | | | [unittests](https://app.codecov.io/gh/mila-iqia/milatools/pull/103/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mila-iqia) | `59.20% <57.87%> (-2.53%)` | :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.

gyom commented 7 months ago

For what it's worth, I'm not familiar with the code and I read the PR in diagonal to get a sense of what was in there. It looked alright in general. The main question that I would have is whether all the work for nice progress bars and whatnot will be obsolete when MFA kicks in? Can users log in manually once, perform the 2FA ritual, and then run this command without having to do it again?

lebrice commented 7 months ago

For what it's worth, I'm not familiar with the code and I read the PR in diagonal to get a sense of what was in there. It looked alright in general. The main question that I would have is whether all the work for nice progress bars and whatnot will be obsolete when MFA kicks in? Can users log in manually once, perform the 2FA ritual, and then run this command without having to do it again?

Yes! The goal of this RemoteV2 is to use the ControlMaster directive of OpenSSH, so that the connection (and 2fa) is only performed once and a socket file is created. The connection is then reused.

lebrice commented 7 months ago

@gyom Here's the flow with 2FA: Notice how after the first interrupted run, there is no need to perform 2FA again:

https://github.com/mila-iqia/milatools/assets/13387299/662b6639-0b72-4de2-aeb3-6cf036dacd35