Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Task directory_out #340

Merged
merged 12 commits into from
Jan 10, 2022
Merged

Task directory_out #340

merged 12 commits into from
Jan 10, 2022

Conversation

DavidGOrtega
Copy link
Contributor

@DavidGOrtega DavidGOrtega commented Dec 29, 2021

Created the nested structure using a TypeSet because TypeMap can not set predefined keys so we were not able in that case to setup the ForceNew strategy.

Example

resource "iterative_task" "tpi-examples-basic2" {
    provider = iterative2

    name      = "tpi-examples-basic2"
    cloud     = "az"
    region    = "us-east"
    machine   = "Standard_D2S_v3"
    #spot      = 0

    workdir {
        input  = "."
        output = "./task2"
    }
    
    script = <<-END
    #!/bin/bash
    echo 'hello tpi' > data.txt
    printenv >> data.txt
    END
}

@DavidGOrtega
Copy link
Contributor Author

DavidGOrtega commented Jan 3, 2022

@iterative/cml what do you think about: directory and directory_out ?

1

In my opinion I prefer to have them this way opposed to a nested structure as

directory : {
in:
out
}

@DavidGOrtega
Copy link
Contributor Author

2

and probably prefer dir and dir_out

@0x2b3bfa0
Copy link
Member

Voting in favor of nested blocks:

resource "iterative_task" "example" {
  ...
  storage {
    input_directory = "."
    output_directory = "output"
    ...
  }
}

@0x2b3bfa0
Copy link
Member

No strong preference either. 🤷🏼‍♂️

@DavidGOrtega
Copy link
Contributor Author

Voting in favor of nested blocks:

Just downvote 1

@casperdcl casperdcl added enhancement New feature or request resource-task iterative_task TF resource labels Jan 3, 2022
@dacbd
Copy link
Contributor

dacbd commented Jan 3, 2022

I think I would be in favor of a storage block, It would be a convenient place to put a potential addition of a pre-made / specified object-storage path

@casperdcl casperdcl added the testing Unit tests & debugging label Jan 4, 2022
@DavidGOrtega
Copy link
Contributor Author

I would not say storage is the best word @iterative/cml

@DavidGOrtega
Copy link
Contributor Author

👆 updated PR desc

Im doing the integration tests in another PR.
I have successfully tested this with:

1 Nothing set

resource "iterative_task" "tpi-examples-basic2" {
    provider = iterative2

    name      = "tpi-examples-basic2"
    cloud     = "az"
    region    = "us-east"
    machine   = "Standard_D2S_v3"
    #spot      = 0

    script = <<-END
    #!/bin/bash
    echo 'hello tpi' > data.txt
    printenv >> data.txt
    END
}

2 directory set then change directory_out just refreshes

resource "iterative_task" "tpi-examples-basic2" {
    provider = iterative2

    name      = "tpi-examples-basic2"
    cloud     = "az"
    region    = "us-east"
    machine   = "Standard_D2S_v3"
    #spot      = 0
    storage {
        directory = "."
    }

    script = <<-END
    #!/bin/bash
    echo 'hello tpi' > data.txt
    printenv >> data.txt
    END
}

and then updating the storage

resource "iterative_task" "tpi-examples-basic2" {
    provider = iterative2

    name      = "tpi-examples-basic2"
    cloud     = "az"
    region    = "us-east"
    machine   = "Standard_D2S_v3"
    #spot      = 0

    storage {
        directory = "."
        directory_out = "./task2"
    }
    
    script = <<-END
    #!/bin/bash
    echo 'hello tpi' > data.txt
    printenv >> data.txt
    END
}

3 directory set then change directory destroys and create

resource "iterative_task" "tpi-examples-basic2" {
    provider = iterative2

    name      = "tpi-examples-basic2"
    cloud     = "az"
    region    = "us-east"
    machine   = "Standard_D2S_v3"

    script = <<-END
    #!/bin/bash
    echo 'hello tpi' > data.txt
    printenv >> data.txt
    END
}

and then updating the storage

resource "iterative_task" "tpi-examples-basic2" {
    provider = iterative2

    name      = "tpi-examples-basic2"
    cloud     = "az"
    region    = "us-east"
    machine   = "Standard_D2S_v3"

    storage {
        directory = "."
        directory_out = "./task2"
    }
    
    script = <<-END
    #!/bin/bash
    echo 'hello tpi' > data.txt
    printenv >> data.txt
    END
}

Co-authored-by: Restyled.io <commits@restyled.io>
@DavidGOrtega DavidGOrtega requested a review from a team January 5, 2022 16:10
@DavidGOrtega DavidGOrtega marked this pull request as ready for review January 5, 2022 16:10
@0x2b3bfa0
Copy link
Member

Doesn't contemplate droste effect prevention nor input exclusion as per #307 (comment), but... rhinoceroses definitely can be farm animals. 😄

@0x2b3bfa0
Copy link
Member

Nitpick: any preference for directory and directory_out instead of input_directory and output_directory or input and output for the sake of brevity? In theory, APIs are forever.

@DavidGOrtega
Copy link
Contributor Author

Nitpick: any preference for directory and directory_out instead of input_directory and output_directory or input and output for the sake of brevity? In theory, APIs are forever.

directory is better than input_directory because is the default thing, probably users won't need the output unless they want to run many times... indeed to me sounds better workdir. I do not like input nor output because they are meaningless inside the storage structure... if we were using directory instead of storage I would agree

@DavidGOrtega
Copy link
Contributor Author

@0x2b3bfa0 please review 🙏

@0x2b3bfa0

This comment has been minimized.

@DavidGOrtega
Copy link
Contributor Author

@0x2b3bfa0 any chance?

Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

Having tested the previous version (just directory) and finding it very painful and counter-intuitive mostly due to the name, I'd strongly suggest workdir:

workdir {
  in(put): ...
  out(put): ...
}

Because the remote instance really uses these as (current) working directories.

@DavidGOrtega
Copy link
Contributor Author

@iterative/cml @casperdcl I feel the same about workdir
Changing it

@DavidGOrtega
Copy link
Contributor Author

@iterative/cml i'm changing just only the interface for the internal test. I we like it II will adapt then the code internally.
I will create a technical_debt issue

@DavidGOrtega
Copy link
Contributor Author

@iterative/cml please review and accept if ok 🙏

@DavidGOrtega
Copy link
Contributor Author

@casperdcl any luck here? 🙏

Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

Just pushed a couple of documentation updates, please review before merge 🙏

@DavidGOrtega
Copy link
Contributor Author

Looks great @casperdcl

@DavidGOrtega DavidGOrtega merged commit be9e0cd into master Jan 10, 2022
@DavidGOrtega DavidGOrtega deleted the task/directory_out branch January 10, 2022 09:20
@casperdcl
Copy link
Contributor

Awesome. Do we have a follow-up issue for some of the outstanding items like #340 (comment)?

@0x2b3bfa0
Copy link
Member

We don't, and we probably should. Or, at least, move it to Notion.

@DavidGOrtega
Copy link
Contributor Author

All solved! We are not going to solve 1,2,3 as ideally we thought because adds complexity and the solution is not right anyway.

Please read the comment why 1,2,3 are discarded in favour of just error if the output folder is not empty

0x2b3bfa0 added a commit that referenced this pull request Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request resource-task iterative_task TF resource testing Unit tests & debugging
Projects
None yet
4 participants