OSC / ondemand

Supercomputing. Seamlessly. Open, Interactive HPC Via the Web
https://openondemand.org/
MIT License
263 stars 99 forks source link

support data-suggest- or data-default- with data-set- #2817

Open robinkar opened 1 year ago

robinkar commented 1 year ago

I happened to find a case where the dynamic form JS (BC_DYNAMIC_JS) does not behave as I would expect it to behave in 3.0, and here is an small example case where it happens. This seems to happen when a field A (project) depends (data-option-for) on field B (reservation), field C (module) depends on field A (project) and sets (data-set) field B (reservation).

In the example config below, setting the project to project_1 with module pytorch makes it impossible to change the reservation, even though only R4 would be an invalid reservation for that particular module and project combination. Meanwhile, the version field, set in the same way as the reservation, is freely selectable. Changing data-option-for-reservation-R4: false on the project field to some value that does not even exist, e.g. data-option-for-reservation-R5: false, or even just data-option-for-reservation-R4: true still causes the same behaviour.

This behaviour seems to occur in 3.0.1, but not in 2.0.32 where it works properly, i.e. reservation freely selectable, but set to correct default value.

Form config example:

# form.yml.erb
attributes:
  project:
    widget: "select"
    options:
      - [
          "project_1",
          data-option-for-reservation-R4: false,
        ]
      - "project_2"
  module:
    widget: "select"
    options:
      - "tensorflow"
      - [
          "pytorch",
          data-set-reservation: "R3",
          data-set-version: "V1",
          data-option-for-project-project-2: false
        ]
  reservation:
    widget: "select"
    options:
      - "R1"
      - "R2"
      - "R3"
      - "R4"
  version:
    widget: "select"
    options:
      - "V1"
      - "V2"
      - "V3"

form:
  - project
  - module
  - reservation
  - version

┆Issue is synchronized with this Asana task by Unito

johrstrom commented 1 year ago

Thanks, I'll take a look.

johrstrom commented 10 months ago

Hi, I'm circling back to this now. I think this is the scenario you describe correct?

steps to replicate:

expected behavior: I can change the reservation from R3 to something different

actual behavior: I cannot change the reservation from R3

my answer Because you've chosen pytorch, we set reservation based on this data-set directive - data-set-reservation: "R3".

Basically you're hitting this bit. You changed something (the reservation) we're trying to propagate that event to other event handlers in case they have some related relationship. Obviously the choice of pytorch is relevant here.

So it's not just that we're handling change events - it's more like when something does change, we may need to change other elements of the form given these secondary relationships like the choice of pytorch forcing R3.

https://github.com/OSC/ondemand/blob/6259e46ac58ab08490098188585d04639480db8a/apps/dashboard/app/javascript/packs/batch_connect.js#L715-L716

All that said - given pytorch's data-set-reservation directive, it seems to me that this is the correct behaviour. That you would want to keep/force R3 when pytorch is chosen. I know it may be different behavior from 2.0 , but does it make sense to keep/force R3 when pythorch is chosen?

robinkar commented 10 months ago

Yes, that does seem to be the correct scenario.

So far we've been using these data-set-* as mostly guiding/helping the user choose the right options, so they still have the possibility of changing them to something else. Due to this we would prefer having it not be forced, so it would work how the version field here (affected by data-set-version) works in both 2.0 and 3.0, although I can see that forcing the values would be equally correct.

johrstrom commented 10 months ago

Due to this we would prefer having it not be forced, so it would work how the version field here (affected by data-set-version) works in both 2.0 and 3.0, although I can see that forcing the values would be equally correct.

Maybe we need a data-suggest- ? That'll set the value once and only once? It's sort of a tough situation because yes you want to set reservation to R3 but only under certain circumstances?

We're mostly using data-set to set hidden values like reservation + account combinations where you choose the account but the reservation is automatically set to something.

Happy to have the conversation about expanding this to suit your needs.

robinkar commented 10 months ago

I feel like the current data-set- option is what my idea of data-suggest- would be, except for this edge case, while the functionality of a forced data-set- attribute can be achieved right now with hidden fields or some logic elsewhere as you mentioned. One case where we use this is an app where the amount of resources is defined for each module using data-set-. These resources options are hidden, but by checking a checkbox the user can show them and choose to override the defaults for the module. In that case we'd like the amount of resources to be populated with the default values for that module, while still allowing changes. If the definition of "once and only once" includes updating those values once the module changes, even though the user may have changed the resource values, that is what we'd like.

In this particular edge case (issue) it seems like checking if the select option value has changed before applying data-set- attributes could solve the problem. I.e. reservation is changed -> project select options are updated (due to data-option-for-reservation), but results in no changes to value or available options -> module options do not need to be updated (which would trigger the data-set-reservation). Not sure how viable this is with the current code though.

johrstrom commented 10 months ago

OK - I'll turn this ticket into a feature request, though I can't put it in 3.1 yet.

How about some scheme like:

data-set will always set & propogate values. data-suggest or data-default (don't know the name yet) will only set it once, allowing users to override.

robinkar commented 10 months ago

That sounds good.