Add "mila-cpu" and "mila-gpu" entries in ~/.ssh/config in mila init #28

lebrice · 2023-01-26T03:05:53Z

Refactor some of the code of the mila init command.
Add a "mila-cpu" and "mila-gpu" entry to ~/.ssh/config if the user wants them.
- "mila-cpu": Asks for a cpu-only job in the unkillable partition, with 2 cpus and 16 Gb of RAM.
- "mila-gpu": Asks for a GPU job in the unkillable partition, with 2 cpus and 16 Gb of RAM.

The main goal of this is to avoid having users of the cluster click the "mila" button in the VSCode Remote Explorer panel, as it connects them to the login node. This way, they now directly have a way to connect to
a compute node.

This is fairly equivalent to using these parameters to mila code:

mila code . --alloc --partition=unkillable --cpus-per-task=2 --mem=16G

Todos:

Write tests. Work in progress: still unable to feed the user inputs to the
questions. I'd need some help with this @breuleux.
~~Make sure that the ssh entries are added in the right place when there are existing entries. Maybe confirm with @olexa?~~
- Very hard / next to impossible to do without having to replicate the ssh-config resolution logic. For now, users might have to reorder some entries.

satyaog · 2023-01-31T19:09:19Z

milatools/cli/commands.py

+            exit("No ssh configuration file was found.")
+        sshpath = cfgpath.parent
+        if not sshpath.exists():
+            sshpath.mkdir(mode=0o700, exist_ok=True)


Does this also chmod the dir if it already exists? Should it as this could also resolve some milatools unrelated issues?

Fixed in 5cade2d

satyaog · 2023-01-31T19:44:39Z

milatools/cli/commands.py

+
+def setup_ssh_config_interactive(
+    ssh_config_path: Union[str, Path] = "~/.ssh/config",
+    _input: Optional[PipeInput] = None,


Soince this is only used for testing, should we instead use unittest.mock.patch to mock the questionary with functools.partials? E.g.

with prompt_toolkit.input.defaults.create_pipe_input() as inp: with unittest.mock.patch("questionary.confirm", new=functools.partial(questionary.confirm, input=inp)): inp.send_text("Y") [...]

Fixed in 5647ea0

satyaog · 2023-01-31T21:00:00Z

milatools/cli/commands.py

+def _confirm_changes(
+    ssh_config: SSHConfig, hosts: list[str], _input: Optional[PipeInput] = None
+) -> bool:
+    print(T.bold("The following code will be appended to your ~/.ssh/config:\n"))


Suggested change

print(T.bold("The following code will be appended to your ~/.ssh/config:\n"))

print(T.bold("The following code will be appended or modified in your ~/.ssh/config:\n"))

since there is potentially a rename on *.server.mila.quebec?

Added this change (not sure in which commit exactly)

satyaog · 2023-01-31T21:15:48Z

tests/cli/test_mila_init_ssh_config.py

+def test_creates_ssh_config_file(tmp_path: Path):
+    ssh_config_path = tmp_path / "ssh_config"
+    with set_user_inputs(["y", "bob\r", "y", "y", "y", "y", "y"]) as input_pipe:
+        setup_ssh_config_interactive(tmp_path / "ssh_config", _input=input_pipe)
+    assert ssh_config_path.exists()


with set_user_inputs(["y", "bob\r", "y", "y", "y", "y", "y"]) as input_pipe: with unittest.mock.patch("questionary.confirm", new=functools.partial(questionary.confirm, input=input_pipe)), \ with unittest.mock.patch("questionary.text", new=functools.partial(questionary.text, input=input_pipe)): setup_ssh_config_interactive(tmp_path / "ssh_config")

?

def test_creates_ssh_config_file(tmp_path: Path, file_regression): [...] with open("ssh_config_path") as _f: file_regression.check(_f.read())

with pytest-regression?

Fixed in 5647ea0 for the first part, and 14f005c + 2b6d6cb for file_regression

satyaog · 2023-01-31T21:25:47Z

tests/cli/test_mila_init_ssh_config.py

+        with open(ssh_config_path) as f:
+            resulting_contents = f.read()
+
+    assert resulting_contents == expected_contents


Suggested change

assert resulting_contents == expected_contents

file_regression.check(resulting_contents)

with pytest-regression?

Fixed in 14f005c

satyaog · 2023-01-31T21:32:58Z

I have some suggestions but I think this is good and could be merged as is (although I would merge it after #32)

satyaog · 2023-01-31T21:50:19Z

tests/cli/test_mila_init_ssh_config.py

+        # If there is already something in the config file, the new entries should be appended at
+        # the end.
+        # TODO: Confirm this with @Olexa
+        expected_contents = initial_contents + _join_blocks(*expected_blocks)


ChatGPT tells me that the first matching entry is used so it depends on the behaviour we look for. If we don't want to mess with existing things, then appending will not break anything. But if we do think we know better, then entries should be added first so they take precedence on what was already existing

Good point. I think it would be quite complicated to try to insert the new entries exactly where they should be, given an existing set of SSH entries, since we'd have to check for matches and everything.
This is why I was looking for Olexa's input as well.

We could also do something that works in ~90% of all cases, and requires the user to manually reorder entries in their .ssh/config the rest of the time. Not sure.

satyaog · 2023-01-31T21:54:10Z

milatools/cli/commands.py

+
+    # Check for a mila entry in ssh config
+    # NOTE: If there is no `mila` entry, ssh_config.host("mila") returns an empty dictionary.
+    username = ssh_config.host("mila").get("user")


This will not catch a Host that could look like

Host mila alias1 [...] User username [...]

even if ssh mila would match that existing configuration

Fixed in e111649

lebrice · 2023-02-01T19:30:15Z

Okay I'm pretty much done playing around with this. Let me know what you think @satyaog @breuleux .

bouthilx · 2023-03-29T15:37:40Z

@satyaog Review needed :)

- Refactor some of the code of the `mila init` command. - Add a "mila-cpu" and "mila-gpu" entry to ~/.ssh/config if the user wants. - "mila-cpu": Asks for a cpu-only job in the unkillable partition, with 2 cpus and 16 Gb of RAM. - "mila-gpu": Asks for a GPU job in the unkillable partition, with 2 cpus and 16 Gb of RAM. The main goal of this is to avoid having users of the cluster click the "mila" button in the VSCode Remote Explorer panel, as it connects them to the login node. This way, they now directly have a way to connect to a compute node. This is fairly equivalent to using these parameters to `mila code`: ``` mila code . --alloc --partition=unkillable --cpus-per-task=2 --mem=16G ``` Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

@breuleux

- Work in progress. Still unable to feed the user inputs to the questions. I'd need some help with this @breuleux. Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

lebrice force-pushed the add_entries_mila_init branch from 8110c13 to cc1419b Compare January 26, 2023 03:09

lebrice changed the title ~~Add entries mila init~~ Add "mila-cpu" and "mila-gpu" entries in ~/.ssh/config in mila init Jan 27, 2023

lebrice marked this pull request as ready for review January 27, 2023 17:17

lebrice changed the base branch from dev to master January 27, 2023 17:26

lebrice force-pushed the add_entries_mila_init branch from 15aa626 to b748c40 Compare January 27, 2023 17:29

lebrice requested a review from breuleux January 27, 2023 20:43

satyaog approved these changes Jan 31, 2023

View reviewed changes

satyaog reviewed Jan 31, 2023

View reviewed changes

lebrice added 18 commits April 4, 2023 16:12

Adding tests for ssh configuration of mila init

a73981b

- Work in progress. Still unable to feed the user inputs to the questions. I'd need some help with this @breuleux. Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Minor tweak to the docstring of setup_ssh_config

b9f72c2

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Add more test cases and improve readability

6174cde

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

[wip] Adjust the prompting and the tests

f75f03e

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Simplify test, redesign prompting: confirm once

0101f45

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Fix issue in test

46e3404

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Rework the mila init prompting, tests pass

6c003ee

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Misc small changes

c8c52fa

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Add more tests for existing entries

9453190

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Remove outdated comment

28cd090

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Add file_regression fixture in test and make files

5111e80

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Remove hard-coded expected values in test

517147f

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Remove _input argument, use fixture instead

e266ffb

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Chmod the .ssh directory if it already exists

c8f86b7

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Also retrieve username in edge-case with aliases

69bb44c

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Add tests for _get_username function

90783af

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Revert code format changes irrelevant to PR

ddbc0f6

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

lebrice and others added 11 commits April 4, 2023 16:12

Move creation of ~/.ssh/config file to a function

29510d8

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Add tests for _setup_ssh_config_file

3bf8d91

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Fixed bugs in _setup_ssh_config_file

20c4f61

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Adjust docstring

35e716f

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Move parts of mila init command body to new module

688523a

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Reduce number of prompts, remove test files

83ac5b9

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Add generated regression files for new tests

909ffe7

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Fix bug in test with python 3.8

4e4d24b

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Fix formatting issues in files

6234494

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Fix import order in commands.py

7859746

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Remove partition/dependency from mila-cpu/gpu

f0f935b

breuleux force-pushed the add_entries_mila_init branch from bed16d8 to f0f935b Compare April 4, 2023 20:33

breuleux added 2 commits April 4, 2023 16:38

Formatting fixes

a8a7907

Fix regression tests

eb1bf45

breuleux merged commit 2d2867b into mila-iqia:master Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "mila-cpu" and "mila-gpu" entries in ~/.ssh/config in mila init #28

Add "mila-cpu" and "mila-gpu" entries in ~/.ssh/config in mila init #28

lebrice commented Jan 26, 2023 •

edited

Loading

satyaog Jan 31, 2023 •

edited

Loading

lebrice Feb 1, 2023

satyaog Jan 31, 2023 •

edited

Loading

lebrice Feb 1, 2023

satyaog Jan 31, 2023

lebrice Feb 1, 2023

satyaog Jan 31, 2023 •

edited

Loading

satyaog Jan 31, 2023 •

edited

Loading

lebrice Feb 1, 2023

satyaog Jan 31, 2023

lebrice Feb 1, 2023

satyaog commented Jan 31, 2023

satyaog Jan 31, 2023

lebrice Feb 1, 2023

satyaog Jan 31, 2023 •

edited

Loading

lebrice Feb 1, 2023

lebrice commented Feb 1, 2023

bouthilx commented Mar 29, 2023

	print(T.bold("The following code will be appended to your ~/.ssh/config:\n"))
	print(T.bold("The following code will be appended or modified in your ~/.ssh/config:\n"))

	assert resulting_contents == expected_contents
	file_regression.check(resulting_contents)

Add "mila-cpu" and "mila-gpu" entries in ~/.ssh/config in mila init #28

Add "mila-cpu" and "mila-gpu" entries in ~/.ssh/config in mila init #28

Conversation

lebrice commented Jan 26, 2023 • edited Loading

satyaog Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satyaog Jan 31, 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

satyaog Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

satyaog Jan 31, 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

satyaog commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satyaog Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebrice commented Feb 1, 2023

bouthilx commented Mar 29, 2023

lebrice commented Jan 26, 2023 •

edited

Loading

satyaog Jan 31, 2023 •

edited

Loading

satyaog Jan 31, 2023 •

edited

Loading

satyaog Jan 31, 2023 •

edited

Loading

satyaog Jan 31, 2023 •

edited

Loading

satyaog Jan 31, 2023 •

edited

Loading