Fix --persist mila code bug and intermittent connection errors [MT-78]
#101
Conversation
Signed-off-by: Fabrice Normandin <[email protected]>
lebrice
changed the title
Signed-off-by: Fabrice Normandin <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #101 +/- ##
==========================================
+ Coverage 61.03% 61.73% +0.69%
==========================================
Files 9 9
Lines 1404 1445 +41
==========================================
+ Hits 857 892 +35
- Misses 547 553 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
lebrice
changed the title
lebrice
changed the title
mila code bug and intermittent connection errors [MT-78]
Signed-off-by: Fabrice Normandin <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
`alloc` needs to be a list of strings, but it was typed as `Sequence[str]`, which allows `str` to be passed (since `str`s are sequences of `str`s). This changes it to `list[str]` which is stricter and correct. Incidentally, there was an undetected bug in the regression test at `tests/integration/test_code_command.py::test_code` because I was passing the allocation flags (a string) as salloc. Signed-off-by: Fabrice Normandin <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
milatools/cli/commands.py
Outdated
| if "pytest" in sys.modules: | ||
| break |
There was a problem hiding this comment.
This is the only change that should be done before merging as this looks like left over from debug
Would this break mila code if someone installs pytest in the same env? Probably better to use an environment variable instead?
| if "pytest" in sys.modules: | |
| break |
There was a problem hiding this comment.
This just checks if it's run from within pytest. This doesn't change anything during normal operation.
There was a problem hiding this comment.
It's not leftover from debug, it's actually just an easy way to change the behaviour of the code when it's being run within pytest. I'll make a small function to make this easier to read.
There was a problem hiding this comment.
Addressed in 6411d30
| # The next line may overflow to two (or maybe even more?) lines if the name of the | ||
| # $HOME dir is too long. |
There was a problem hiding this comment.
It does? That's a shame :( But this could mean a quota could also be splitted in two line right having
104
8576
instead of 1048576?
There was a problem hiding this comment.
Words aren't split, but what happens is that the home folder is on a single line and the rest on another line.
This here would also work the quotas somehow needed more than one line to display, also.
Co-authored-by: satyaog <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
Signed-off-by: Fabrice Normandin <[email protected]>
mila codewith--persistfails! #100cd $SCRATCHbeforesallocandsbatchon the mila clusterbanner_timeout: 60in theconnect_kwargsthat are passed from fabric toparamiko.SSHClient.connectfor the mila cluster when we create amilatools.cli.remote.Remoteobject. This is the most "experimental" change that might require some more testing. In my experience it consistently solved a new paramiko "Unable to parse SSH banner" error that I started experiencing during tests.