seqeralabs / tower-cli

Nextflow Tower CLI tool
Apache License 2.0
42 stars 9 forks source link

Add support for Fusion v2 at AWS Batch CE creation #330

Closed ejseqera closed 1 year ago

ejseqera commented 1 year ago

Currently, when using tw compute-envs add aws-batch forge, the use of the --fusion flag enables usage of the deprecated Fusion v1 mounts feature on Tower. There is no way to enable Fusion v2 via compute-envs add, similarly Wave (related to #280) and enable use of NVMe instances.

 --fusion                                   With Fusion enabled, S3 buckets specified in the Pipeline work directory and Allowed S3 Buckets fields will be
                                                   accessible in the compute nodes storage using the file path /fusion/s3/BUCKET_NAME.                                         

This means creation of an AWS batch CE with Fusion v2/Wave has to be done via a JSON and tw compute-envs import. It would be nice to use the add method using --fusion (or --fusionv2), --wave, and --nvme or something similar to replicate the toggles on the UI.

pditommaso commented 1 year ago

+1

jordeu commented 1 year ago

I'd rename the current --fusion flag to --fusion-v1 and then use --fusion, --wave and --fast-storage to match same GUI naming.

pditommaso commented 1 year ago

Changing the sematic of the --fusion can create confusion depending on the cli version used. I'd say to use --fusion-v2. Agree on --fast-storage and --wave

jordeu commented 1 year ago

Given that we consider Fusion v1 deprecated I don't like the idea to add a --fusion-v2 flag. We will need to stick with it "forever".

Possible alternatives:

  1. When they enable --wave then we assume that --fusion means v2. If not means v1 as it is now.
  2. Instead of --fusion-v2 we add a flag --fusion-wave that means to enable Fusion v2 and Wave (the --wave will still exists but won't be needed if you add --fusion-wave).

I prefer option one because our goal is to deprecate Fusion v1.

gwright99 commented 1 year ago

Agree with @pditommaso - IMO ignore the delivery mechanism and focus on clearly defining the binary version (i.e. --fusion-v2).

This is particularly relevant as the continued presence of Fusion v1 in Tower screens / documentation still catches new clients who don't yet appreciate the nuance. I suspect you'll find the same to be true for newbie CLI users too.

jordeu commented 1 year ago

If we're doing this to maintain backward compatibility (even if we're in a "beta" Tower CLI version and Fusion v1 is "deprecated"), I can understand.

However, if the aim is to avoid confusion, I can't agree. The real confusion arises from advertising all Fusion v2 features and then having an older version, also named Fusion, that's completely different. Perhaps we should consider renaming the old version?

Overall, my first alternative (enabling Fusion v2 when --wave is enabled and v1 when it's not) seems the most balanced solution. Does it introduce confusion? No, it merely reflects the existing confusion, but it does leave the door open to resolve that confusion if we decide to remove Fusion v1 in the future.

jordeu commented 1 year ago

Or (if Fusion v1 is really deprecated) then we disable the option to create new CE from Tower CLI that uses Fusion v1 and --fusion flag starting from the next release is just going to enable always Fusion v2. If you want to use deprecated features, use a deprecated Tower CLI version.

pditommaso commented 1 year ago

When using --fusion the CLI shold show a warning e.g. "This feature is deprecated consider using Fusion v2 select '--fusion-v2' option`

jordeu commented 1 year ago

For me feels very strange to use the version v2 as a suffix to name the flag. I've never seen that on a CLI. What are we going to do if we have a version 3?

pditommaso commented 1 year ago

we should have called with a different name in the first place. I don't see a big problem using --fusion-v2, it's just a string the important thing that's readable (maybe wit can be added a shortcut -F)?

swampie commented 1 year ago

Let's try to unlock the conversation as there are customers (@drpatelh above all) waiting for this:

1) enable fusion v2 by using the flag --fusion-v2 and add --nvme and --wave I would go with that one as at some point we can deprecate --fusion in favour of the v2.

2) Enable fusion v2 by default when using --wave I prefer an explicit approach: once all this features will be in the CLI we can produce documentation and example that will make the usage clear and straightforward.

Let's move forward with this

pditommaso commented 1 year ago

Enable fusion v2 by default when using --wave

nope. you may want to use wave without fusion and fusion without wave

swampie commented 1 year ago

Enable fusion v2 by default when using --wave

nope. you may want to use wave without fusion and fusion without wave

exactly what I said in the comment above: but thanks for highlighting

gwright99 commented 1 year ago

Let's try to unlock the conversation as there are customers (@drpatelh above all) waiting for this:

  1. enable fusion v2 by using the flag --fusion-v2 and add --nvme and --wave I would go with that one as at some point we can deprecate --fusion in favour of the v2.
  2. Enable fusion v2 by default when using --wave I prefer an explicit approach: once all this features will be in the CLI we can produce documentation and example that will make the usage clear and straightforward.

Let's move forward with this

IMO explicit is always better. A tiny bit more typing in return for much easier readability / reduced cognitive load when trying to figure out what someone did.

jordeu commented 1 year ago

Well this is less explicit because people is going to select only --fusion-v2 expecting that is enough but it is going to fail unless they select --wave. So for me it is more explicit that as we do on the website only selecting --fusion-v2 also enables wave, and in the case that you do not want wave you need to be more explicit with a --fusion-v2 --no-wave.

Also what I do not understand is that it seems that in a future we are going to remove Fusion v1 and then --fusion will mean Fusion v2, isn't it? If this is the case, why don't we do this change now and just add the --fusion-v1 flag for people that still explicitly wants to use the deprecated version?

For me to now introduce --fusion-v2 and in a future change the behaviour of --fusion is just to add more confusion and also to invite more users to use Fusion v1.

I'm just following the principle to try to minimize the flags to express what more users need (to have good defaults). https://clig.dev

JaimeSeqLabs commented 1 year ago

I'm thinking that maybe there is a middle ground.

Given that the following are mutually exclusive flags:

Using that we manage:

And in the future we can fully deprecate v1 by removing --fusion-version and making --fusion enable v2 by default OR We can remove --fusion while keeping --fusion-version which may be handy if there is ever a fusion v3.

drpatelh commented 1 year ago

I like @JaimeSeqLabs suggestions.

Is anyone actually still using Fusion V1? Wondering whether we just hard deprecate it in this release of the Tower CLI? And then use --fusion to set Fusion V2. Are we going to maintain multiple versions of Fusion in the future? If not, there is no point in having a --fusion-version parameter.

pditommaso commented 1 year ago

Fusion v1 is deprecated and planned for removal in this quarter.Same for EBS autoscale.

Let's unlock this thread:

  1. use --OPTION, and --no-OPTION as Jordi is suggesting being the current in-place
  2. add --wave to enable Wave feature
  3. add --fusion-v2 to enable Fusion feature
  4. add --fast-storage to enable NVMe formatting
  5. change --fusion returning an error and suggesting the user to use --fusion-v2
  6. no implicit default across fusion and wave options
  7. no mixed options e.g. --wave-fusion

What am I missing?

jordeu commented 1 year ago

What am I missing?

Why we use --fusion-v2. From the plan says "Q3: remove from the UI, still keep operating for existing CEs". Tower CLI is just another UI of Tower. Why we do not remove it now if it is in the plan?

pditommaso commented 1 year ago

Because if you remove it, you will break the CLI for users who might use it without proper messaging. Also, fusion-v2 naming is aligned with the API and UI usage.