search

iterative / terraform-provider-iterative

☁️ Terraform plugin for machine learning workloads: spot instance recovery & auto-termination | AWS, GCP, Azure, Kubernetes
https://registry.terraform.io/providers/iterative/iterative/latest/docs
Apache License 2.0
288 stars 27 forks source link

Provide a standalone command-line tool #610

Closed 0x2b3bfa0 closed 2 years ago

0x2b3bfa0 commented 2 years ago

Changes

25 files are a lot of files, acknowledged

Stability

Zero API stability guarantees; use under your own responsibility, dear repository stalker. 😄

Usage

_I is what remains when you remove Terraform and the Provider from “TPI”. :smilingimp: To try it, clone this branch, run make and alias I=./terraform-provider-iterative for the sake of simplicity.

Basic commands are:

  • I create to create a new task,
  • I read <id> to see its status,
  • I delete <id> to delete it and
  • I list to see the existing tasks.

Likewise, if you’re stuck, I help as much as you want.

Options can be specified from the command line (e.g. --cloud=aws to specify the cloud provider) or from a main.tf file in the current working directory; this file is fully[^1] complatible with the previous Terraform syntax.

Possible next steps

[^1]: Unlike in Terraform HCL, values must not contain arithmetic expressions (e.g. 60*60*24 is not valid, but 86400 is valid). Also, declaring multiple resource blocks produces undefined behavior; i.e. defined behavior, but too contrived to explain in a footnote; definitely not what users expect.

0x2b3bfa0 commented 2 years ago

@casperdcl, you may want to convert the following commit comments into review comments:

dacbd commented 2 years ago

using the getting started example from a clean directory worked (detected /picked up the main.tf).... but

tpi create doesn't print a name and...

without names tpi list is very unhelpful

dabarnes@ubuntu:~/iterative/test$ ./tpi list
Reading configuration from /home/dabarnes/iterative/test/main.tf
INFO[0000] 131g8rpkllfpo                                
INFO[0000] 1kd54081mmzyg                                
INFO[0000] 2f550k07u12md                                
INFO[0000] 2gmf31ldp1ekz                                
INFO[0000] 3df4fovpzq3ug                                
INFO[0000] 3uopp06uuy1sq                                
INFO[0000] fo9ju66nxqui                                 
INFO[0000] hd4a0ax36vpj                                 
INFO[0000] pgu446r33owk                                 
INFO[0000] smoke-test-2551825938

I can't use tpi read from the same dir

dabarnes@ubuntu:~/iterative/test$ ./tpi create
Reading configuration from /home/dabarnes/iterative/test/main.tf
INFO[0000] Creating resources...                        
INFO[0000] [1/12] Parsing PermissionSet...              
INFO[0000] [2/12] Importing DefaultVPC...               
INFO[0001] [3/12] Importing DefaultVPCSubnets...        
INFO[0001] [4/12] Reading Image...                      
INFO[0002] [5/12] Creating Bucket...                    
INFO[0003] [6/12] Creating SecurityGroup...             
INFO[0006] [7/12] Creating KeyPair...                   
INFO[0008] [8/12] Reading Credentials...                
INFO[0008] [9/12] Creating LaunchTemplate...            
INFO[0009] [10/12] Creating AutoScalingGroup...         
INFO[0012] [11/12] Uploading Directory...               
INFO[0012] Reading resources... (this may happen several times) 
INFO[0012] [1/9] Reading DefaultVPC...                  
INFO[0012] [2/9] Reading DefaultVPCSubnets...           
INFO[0012] [3/9] Reading Image...                       
INFO[0014] [4/9] Reading Bucket...                      
INFO[0014] [5/9] Reading SecurityGroup...               
INFO[0014] [6/9] Reading KeyPair...                     
INFO[0016] [7/9] Reading Credentials...                 
INFO[0016] [8/9] Reading LaunchTemplate...              
INFO[0016] [9/9] Reading AutoScalingGroup...            
INFO[0016] Read completed                               
INFO[0016] Transferring 151.8MB (2 files)...            
INFO[0026]    13.185 MiB / 144.721 MiB, 9%, 1.323 MiB/s, ETA 1m39s 
INFO[0036]    27.466 MiB / 144.721 MiB, 19%, 1.365 MiB/s, ETA 1m25s 
INFO[0046]    41.716 MiB / 144.721 MiB, 29%, 1.416 MiB/s, ETA 1m12s 
INFO[0056]    56.060 MiB / 144.721 MiB, 39%, 1.401 MiB/s, ETA 1m3s 
INFO[0066]    69.810 MiB / 144.721 MiB, 48%, 1.378 MiB/s, ETA 54s 
INFO[0076]    81.997 MiB / 144.721 MiB, 57%, 1.312 MiB/s, ETA 47s 
INFO[0086]    92.372 MiB / 144.721 MiB, 64%, 1.177 MiB/s, ETA 44s 
INFO[0096]   105.622 MiB / 144.721 MiB, 73%, 1.233 MiB/s, ETA 31s 
INFO[0106]   119.966 MiB / 144.721 MiB, 83%, 1.327 MiB/s, ETA 18s 
INFO[0116]   134.216 MiB / 144.721 MiB, 93%, 1.370 MiB/s, ETA 7s 
INFO[0125] [12/12] Starting task...                     
INFO[0125] Creation completed                           
dabarnes@ubuntu:~/iterative/test$ ./tpi read
Reading configuration from /home/dabarnes/iterative/test/main.tf
Error: accepts 1 arg(s), received 0
Usage:
  task read <name> [flags]

Flags:
  -h, --help   help for read

Global Flags:
      --cloud string    cloud provider
      --log string      log level (default "info")
      --region string   cloud region (default "us-east")

dabarnes@ubuntu:~/iterative/test$
0x2b3bfa0 commented 2 years ago

I create doesn't print a name and... without names I list is very unhelpful

Solved with https://github.com/iterative/terraform-provider-iterative/pull/610/commits/88cede1d2ac0b0a9d1f2e663d0e06d2ed0f14a56 and, perhaps, you can try I create --name=my-experiment-$(uuidgen) to set human-readable names.

0x2b3bfa0 commented 2 years ago

I can't use I read from the same dir

You should use I read <name> where <name> can be retrieved with the I list command. There is no way for I to guess which task you want to read because there is no local state.

It's either “read all” or “read chosen” but there is no brain reading involved. 🧠🔌

dacbd commented 2 years ago

After meticulously removing different files and the lines from my original main.tf I have determined that if you have a timeout = int in your main.tf it fails

0x2b3bfa0 commented 2 years ago

Only if it's an expression, as per the footnote on https://github.com/iterative/terraform-provider-iterative/pull/610#issue-1282657545, right? E.g. timeout = 86400 works but timeout = 24*60*60 doesn't. 🤔

dacbd commented 2 years ago

Only if it's an expression, as per #610 (comment), right?

🙃 correct

0x2b3bfa0 commented 2 years ago

That's why this pull request updates the documentation to use 86400 everywhere... ~until I figure out what magic to do upstream for it to work.~

It turns out that viper uses HCLv1 instead of HCLv2.

dacbd commented 2 years ago

delete states that it read from the config, being "stateless" does read from the config/need anything from it?

dabarnes@ubuntu:~/iterative/test$ ./tpi delete tpi-oktxx4d9io-204zeiwa-1up1spvq
Reading configuration from /home/dabarnes/iterative/test/main.tf
INFO[0000] Deleting resources...                        
INFO[0000] [1/8] Downloading Directory...               
INFO[0000] Reading resources... (this may happen several times) 
INFO[0000] [1/9] Reading DefaultVPC...                  
INFO[0000] [2/9] Reading DefaultVPCSubnets...           
INFO[0000] [3/9] Reading Image...                       
INFO[0000] [3/8] Deleting AutoScalingGroup...           
INFO[0083] [4/8] Deleting LaunchTemplate...             
INFO[0083] [5/8] Deleting KeyPair...                    
INFO[0084] [6/8] Deleting SecurityGroup...              
INFO[0084] [7/8] Reading Credentials...                 
INFO[0084] [8/8] Deleting Bucket...                     
INFO[0085] Deletion completed                           
dabarnes@ubuntu:~/iterative/test$
dacbd commented 2 years ago

also there is some fun counting going on there x/8 or x/9 😀

0x2b3bfa0 commented 2 years ago

delete states that it read from the config, being "stateless" does read from the config/need anything from it?

Yes, it needs a few insignificant details like the cloud provider and the region... Although you can provide them manually (e.g. pass the --region option) and not use a configuration file.

dacbd commented 2 years ago

delete states that it read from the config, being "stateless" does read from the config/need anything from it?

Yes, it needs a few insignificant details like the cloud provider and the region... 175989741-eec3da25-cd26-44c8-87c0-9b07565f84ae.gif Although you can provide them manually (e.g. pass the --region option) and not use a configuration file.

ie the "global" flags?/.../:

dabarnes@ubuntu:~/iterative/test$ ./tpi --help
Reading configuration from /home/dabarnes/iterative/test/main.tf
Task is a command-line tool that allows
    data scientists to run code in the cloud.

Usage:
  task [command]

Available Commands:
  completion  Generate the autocompletion script for the specified shell
  create      Create a task
  delete      Delete a task
  help        Help about any command
  list        List tasks
  read        Read the status of a task

Flags:
      --cloud string    cloud provider
  -h, --help            help for task
      --log string      log level (default "info")
      --region string   cloud region (default "us-east")

Use "task [command] --help" for more information about a command.
0x2b3bfa0 commented 2 years ago

also there is some fun counting going on there x/8 or x/9 😀

Nobody Expects the Spanish Inquisition! 😅 Ideally, those numbers should be calculated algorithmically (https://github.com/iterative/terraform-provider-iterative/issues/290) to avoid off-by-many issues. Anyway, not directly related to this pull request; probably worth a separate discussion.

0x2b3bfa0 commented 2 years ago

The is code for I stop but it doesn't do anything / cmd not found

Argh! Sorry, having the bad habit of using init() to automatically register commands, I forgot to add them. Fixed with https://github.com/iterative/terraform-provider-iterative/pull/610/commits/48180755d8e4e3416e20618eabe7089179c225ac.

0x2b3bfa0 commented 2 years ago

There is a theme of stdout vs stderr I list, I read, I delete do "nothing" from this pov (delete is probably fine though?)

Can you please elaborate?

0x2b3bfa0 commented 2 years ago

Can we present this better? like status: waiting for instance / instance running?

Sounds like you may have overlooked the very last line of I read (?) 🤔 It displays colorful status information like queued/running/failed/succeeded.

Captura de pantalla 2022-06-29 a las 1 17 04

dacbd commented 2 years ago

There is a theme of stdout vs stderr I list, I read, I delete do "nothing" from this pov (delete is probably fine though?)

Can you please elaborate?

sorry I explained this better in my first version of this review before it was violently lost 😞 ...

from the point of view of these commands being able to nicely work as part of a larger system most output is all sent to stderr ./tpi read tpi-3u75pr8sir0hd-t6802j39-3qwi6brx 2>/dev/null

or perhaps something simpler you can see what I feel is counterintuitive:

tpi read [name] > [name].log
wc --bytes [name].log

to additionally elaborate, the log parsing is inconsistent for machine parsing, and actually playing with it more I'm not even sure what weirdness this logging library is trying to do...

I cant go ./tpi read tpi-3u75pr8sir0hd-t6802j39-3qwi6brx |& awk 'match($0, /LOG 0 >> /){print $0}' > my_scripts_logs.log and have this do what I would expect.

I would prefer for equal treatment in the interpretation of \n note the difference between ./tpi read [name] vs ./tpi read [name] |& cat and this is a frivolous use of cat because of the above note where ./tpi read [name] > file results in an empty file.

0x2b3bfa0 commented 2 years ago

I wholeheartedly agree:

  1. Command output differs between teletypes and otherwise
    • E.g. running command on your terminal is different to running command |& cat
    • None of the two formats are good for scripting
  2. Command output goes to standard error, making parsing a lot more difficult
  3. There is a lot to improve; e.g. be able to output just raw logs, or just the status, ...

Not sure if that's on scope for this pull request, but is definitely worth addressing before making this command-line tool generally available-

0x2b3bfa0 commented 2 years ago

Next steps, copied from https://github.com/iterative/terraform-provider-iterative/pull/610#issue-1282657545

[^1]: With a special case for $parallelism > 1$ where users can filter logs for a single task.

DavidGOrtega commented 2 years ago

@0x2b3bfa0 happy to merge?

0x2b3bfa0 commented 2 years ago

@DavidGOrtega :shipit: