workfloworchestrator / orchestrator-core

The workflow orchestrator core repository
Apache License 2.0
42 stars 15 forks source link

Choice with str label and dict value #153

Closed gstewart86 closed 2 years ago

gstewart86 commented 2 years ago

Hey gang,

It appears I'm unable to create a Choice with a str label and dict value. Given the following:

EsdbCircuitEnum = Choice("EsdbCircuitChoice", zip(esdb_circuit_map.values(), esdb_circuit_map.keys()))

I see the error:

ns
    EsdbCircuitEnum = Choice("EsdbCircuitChoice", zip(esdb_circuit_map.values(), esdb_circuit_map.keys()))
  File "/usr/local/lib/python3.9/enum.py", line 386, in __call__
    return cls._create_(
  File "/usr/local/lib/python3.9/enum.py", line 509, in _create_
    classdict[member_name] = member_value
  File "/usr/local/lib/python3.9/enum.py", line 97, in __setitem__
    if _is_private(self._cls_name, key):
  File "/usr/local/lib/python3.9/enum.py", line 49, in _is_private
    and name.startswith(pattern)
AttributeError: 'dict' object has no attribute 'startswith'

The pattern we've adopted in the meantime involves creating a map and translating the user choice like so:

    select_map: Dict = {}
    for edge in available_edges:
        select_map[edge.get('name')] = edge

    ServiceEdgeChoice = Choice("ServiceEdgeChoice", zip(select_map.keys(), select_map.keys()))

    class L2VPNForm(FormPage):
        class Config:
            title = product_name

        service_edge: ServiceEdgeChoice

    form_data = yield L2VPNForm
    user_input = form_data.dict()

    esdb_service_edge = get_resource_by_id(
        "service_edge", select_map[user_input["service_edge"]]["id"], params={"detail": 1}
    )

How does SURFnet's implementation typically handle these cases? Do you have any suggestions for making Choice capable of carrying a dict? It would be extremely nice for example to have a class like APIChoice that accepts parameters to generate that Choice from an external API endpoint, and return the related data automatically when chosen like so

    class L2VPNForm(FormPage):
        class Config:
            title = product_name

        service_edge: APIChoice('service_edge')
Mark90 commented 2 years ago

Hi Stewart,

We handle it the same as you do; we keep the dictionary in the backend and only put corresponding keys in the form values. In general when working with forms the frontend displays a set of options to choose from, with the label being "humanly readable" and the value a compact and unique identifier, which the backend knows how to relate to a more complicated data structure (such as a dictionary). I don't really see the benefit of sending larger data structures to the frontend, but perhaps I don't understand the full extent of your usecase.

On a related note; when using Choice the frontend does not currently submit values, it submits the label instead - which is wrong and inconvenient as we then have to derive the value from the label. This is a bug which Rene has a proof of concept fix for in #157. Maybe this will also improves things for you.

Mark

acidjunk commented 2 years ago

It's not always wrong: for simple choices the label will be often the same as the value. So both will probably have their use-cases. But indeed; it would be very convenient to have access to the value also.

ChoiceValue -> will do just that. Shows the label in a select and submit the value upon submit. Working on some test for it now..

I implemented it as a new form element to ensure that it doesn't break Forms/Workflows.

acidjunk commented 2 years ago

Whilst implementing tests I found that the Choice has an undocumented feature:

class LegChoiceLabel(Choice):
        Primary = ("Primary", "Primary LP")
        Secondary = ("Secondary", "Secondary LP")
        Tertiary = "Tertiary"

Will result in this schema:

"LegChoiceLabel": {
    "description": "An enumeration.",
    "enum": ["Primary", "Secondary", "Tertiary"],
    "options": {"Primary": "Primary LP", "Secondary": "Secondary LP", "Tertiary": "Tertiary"},
    "title": "LegChoiceLabel",
    "type": "string",
},

The form will render -> "Secondary LP" but submit "Secondary"

I'm not sure how that will work with a dict; but you could probably also just have the dict in a var -> and only use the keys as a Choice(strEnum) options -> after the submit you can use the key to retrieve the dict values again.

acidjunk commented 2 years ago

For most fields that do external API requests we implement a custom field in the frontend. This also allows use to implement stuff like "smart" auto complete. An example can be found here: https://github.com/workfloworchestrator/orchestrator-core-gui/blob/main/src/custom-surf/uniforms/ImsNodeIdField.tsx

Another benefit: it will render the form; whilst loading the external API request.

gstewart86 commented 2 years ago

@Mark90

I don't really see the benefit of sending larger data structures to the frontend, but perhaps I don't understand the full extent of your usecase.

It's less about sending a large data structure to the front end (agreed, I don't see a reason to do so), but more about simplifying the generate_choice_map -> provide_user_choice -> resolve_user_choice_using_map pattern. If a dict could be provided, all choice resolution happens within the Form yield statement, preventing developers from having to do that round-trip, choice resolution dance in the first place. You could, in that case, pass user_input directly into State and continue the workflow without post-processing. You make a good point though, I didn't think about the fact that this kind of pattern would result in a large amount of data available on the frontend, which might carry its own set of problems.

@acidjunk Thank you for your feedback as well, I'll see what I can do using the tuples like you describe. We currently have a pattern where we avoid hard-coding any values within the workflows by defining the choice like so:

class NSOBackboneLinkIGPPreference(Choice):
    CORE = "core"
    WORST = "worst"
    TAIL = "tail"
    SEA_CABLE = "sea-cable"
    PREFERRED = "preferred"

and then in a workflow, for example, we can use

if x in [NSOBackboneLinkIGPPreference.CORE, NSOBackboneLinkIGPPreference.TAIL]:
  do something

in order to avoid hard-coding a string. Making those values into a tuple makes that a bit more difficult so it'll require a bit of experimentation on my part to see what fits best.

Another problem on the horizon for us (and maybe worthy of its own Issue), is that the API environment is fairly hostile today, requiring the user to know exactly how the human-readable string is composed on the frontend before making an API call. This seemingly prevents client library auto-generation. It occurs to me just now that, if the value was a dict, it would not resolve this issue, and might make it worse.

This is great discussion/feedback, thanks everyone! I think I've got what I need from this issue. I'll leave it open for a little while in case anyone has any further points.