Skip to content

Conversation

@karajan1001
Copy link
Contributor

@karajan1001 karajan1001 commented Sep 13, 2021

fix #6480

  1. add new command dvc machine list
  2. complete dvc machine modify
  3. add new unit tests for this two commands to ensure the call.
  4. add functionality tests for two commands

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

1. add new command `dvc machine list`
2. complete `dvc machine modify`
3. add new unit tests for this two commands to ensure the call.
4. add functionality tests for two commands
@karajan1001 karajan1001 requested a review from a team as a code owner September 13, 2021 14:14
@karajan1001 karajan1001 requested a review from efiop September 13, 2021 14:14
@karajan1001
Copy link
Contributor Author

documentation is still in progress.

@karajan1001 karajan1001 requested review from pmrowla and removed request for efiop September 13, 2021 14:17
@pmrowla
Copy link
Contributor

pmrowla commented Sep 14, 2021

@karajan1001 tests are failing in windows

______________________________ test_machine_list ______________________________
[gw0] win32 -- Python 3.6.8 C:\hostedtoolcache\windows\Python\3.6.8\x64\python.exe

tmp_dir = WindowsTmpDir('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/popen-gw0/test_machine_list0')
dvc = Repo: 'C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\popen-gw0\test_machine_list0'
capsys = <_pytest.capture.CaptureFixture object at 0x00000172DF2D9DD8>

    def test_machine_list(tmp_dir, dvc, capsys):
        (tmp_dir / ".dvc" / "config").write_text(full_config_text)
    
        assert main(["machine", "list"]) == 0
        cap = capsys.readouterr()
        assert "cloud=azure" in cap.out
    
        assert main(["machine", "list", "foo"]) == 0
        cap = capsys.readouterr()
        assert "cloud=azure" not in cap.out
        assert "cloud=aws" in cap.out
        assert "region=us-west" in cap.out
        assert "image=iterative-cml" in cap.out
        assert "name=iterative_test" in cap.out
        assert "spot=True" in cap.out
        assert "spot_price=1.2345" in cap.out
        assert "instance_hdd_size=10" in cap.out
        assert "instance_type=l" in cap.out
        assert "instance_gpu=tesla" in cap.out
        assert "ssh_private=secret" in cap.out
>       assert f"startup_script={tmp_dir}/.dvc/../start.sh" in cap.out
E       AssertionError: assert 'startup_script=C:\\Users\\runneradmin\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-0\\popen-gw0\\test_machine_list0/.dvc/../start.sh' in 'cloud=aws\nregion=us-west\nimage=iterative-cml\nname=iterative_test\nspot=True\nspot_price=1.2345\ninstance_hdd_size=...unneradmin\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-0\\popen-gw0\\test_machine_list0\\.dvc\\..\\start.sh\n'
E        +  where 'cloud=aws\nregion=us-west\nimage=iterative-cml\nname=iterative_test\nspot=True\nspot_price=1.2345\ninstance_hdd_size=...unneradmin\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-0\\popen-gw0\\test_machine_list0\\.dvc\\..\\start.sh\n' = CaptureResult(out='cloud=aws\nregion=us-west\nimage=iterative-cml\nname=iterative_test\nspot=True\nspot_price=1.2345\n...n\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-0\\popen-gw0\\test_machine_list0\\.dvc\\..\\start.sh\n', err='').out

karajan1001 and others added 3 commits September 14, 2021 15:39
Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>
Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>
1. capitalize global variables.
2. remove precision restriction.
3. solve windows path seperator problem.
@pmrowla
Copy link
Contributor

pmrowla commented Sep 15, 2021

This is a good start and works for now while the feature is in active development. A couple things we will want to clean up eventually before a formal release:

Figure out how the best way to handle terraform/TPI errors - right now if you misconfigure something (i.e. your combination of options are rejected by AWS/Azure) you get a real ugly mess:

Plan: 1 to add, 0 to change, 0 to destroy.
iterative_machine.peter-test: Creating...
╷
│ Error: Failed creating the machine: Not able to decode: InvalidParameterValue: Value (0.000010) for parameter price is invalid. "0.000010" is an invalid spot instance price
│       status code: 400, request id: 3b82fe3f-e82f-420c-9244-2de0c77b5113
│
│   with iterative_machine.peter-test,
│   on main.tf.json line 22, in resource.iterative_machine.peter-test:
│   22:       }
│
╵
ERROR: unexpected error - Cmd 'terraform {cmd}' failed

The existing config list output format for machine list probably won't work for a formal release (it gets very hard to read if you want to configure multiple machines

$ dvc machine list
peter-test.spot_price=0.00001
peter-test.spot=true
peter-test.cloud=aws
peter-test.instance_type=t2.micro
peter-test-large.cloud=aws

But since there's no real ideas on what the output should look like yet, we can leave it as-is for now. I'm not really sure if we want to show the detailed options in machine list or should we just show a single peter-test aws line for each configured machine the way we do for remote list.

@dberenbaum

@pmrowla pmrowla merged commit 18d147e into treeverse:master Sep 15, 2021
cli_args = parse_args(["machine", "list", "foo"])
assert cli_args.func == CmdMachineList
cmd = cli_args.func(cli_args)
m = mocker.patch.object(ui, "write", autospec=True)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use capsys?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No particular reason, just used to use mocker in a unit test and capsys in a functional one.

@dberenbaum
Copy link
Contributor

I think the output of machine list needs to look like a table with machine names on one axis and different configuration options on the other. @skshetry might also have input about guidelines for this.

@karajan1001 karajan1001 deleted the fix6480 branch September 16, 2021 06:26
@efiop efiop added the feature is a feature label Sep 17, 2021
@dberenbaum
Copy link
Contributor

@karajan1001 Another minor UI tweak is that we need to make clear that the positional arguments are optional and all machines will be shown by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature is a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

machine: implement list/modify configuration commands

5 participants