wildlife-dynamics / ecoscope-workflows

An extensible task specification and compiler for local and distributed workflows.
https://ecoscope-workflows.readthedocs.org/
BSD 3-Clause "New" or "Revised" License
4 stars 2 forks source link

Support setting literal values in yaml spec `with` block? #102

Open cisaacstern opened 2 months ago

cisaacstern commented 2 months ago

Dependencies:

From the original discussion:

@atmorling yes good observation. You are correct that the create map and create plot tasks are nearly identical, with the exception of setting widget_type="map" or widget_type="plot". And therefore could be combined into a single create_html_widget_single_view as long as widget_type (with options "map" or "plot") was also a parameter. If this ultimately seems better, I'm open to considering this. The reason I've kept them separate (at the cost of offending DRY sensibilities 😅 ) is to keep the compilation spec a little more descriptive, i.e.

https://github.com/wildlife-dynamics/ecoscope-workflows/blob/4f63f8cef8b61f2cf9e473ce426b3aba0534d449/examples/compilation-specs/time-density.yaml#L11-L16

This ensures that the widget resulting from create_map_widget_single_view will have the correct value of widget_type="map" (since it's hardcoded into that task).

If we instead had

draw_ecomap: 
  geodataframe: calculate_time_density 
persist_text: 
  text: draw_ecomap 
create_html_widget_single_view: 
  data: persist_text 

Then that would require the user to know that they would also have to set widget_type="map" as a parameter. Currently, that happens in a totally different place (the web form) which seemed very odd). But having gotten to this point in my explanation (and I truly didn't anticipate getting here, so thanks for the question!) ... I'm now realizing that once #90 is finished, we have a path to setting explicit/literal values (that is, not just results from other tasks) in the yaml spec directly, so we could then do something like:

- name: Persist HTML Text for Map
  id: save_map_html
  task: persist_text 
  with:
    text: ${{ ecomap.return }}
- name: Create Map Widget
  id: create_map_widget
  task: create_html_widget_single_view 
  with:
    widget_type: "map"
    data: ${{ save_map_html.return }}

Most of this ☝️ (name/id/task/with field names, etc.) is already encoded in #90. The new thing here is the idea that a literal value ("map") could also be set in the spect itself. I've been wondering if we would encounter situations where we'd want to do this, but just hadn't run into one yet. This seems like a good motivator for that. What do you think? I'll open an issue to track this.

_Originally posted by @cisaacstern in https://github.com/wildlife-dynamics/ecoscope-workflows/pull/91#discussion_r1693235287_

atmorling commented 2 months ago

Apologies if my comment seemed a bit nitpicky, I'm mostly trying to confirm my own understanding of this.

I guess my main thought was that outside of potential differences in default display parameters (width/height/etc), there doesn't seem to be a need to distinguish between map and plot widgets. If those defaults are a good enough reason to have some kind of distinguishing flag, I actually think naming it in the task (as it is in #91) is nicer vs a widget_type flag within the task spec, as you say it's more descriptive. The concern in either case is what I described where you could provide map html to a plot widget (or vice versa) and it would be 'legal' within the spec.

Could we handle it at a type level? For example defining map_html_output and plot_html_output types (both effectively just html strings) from their respective draw_ tasks, that a task like persist_text would subsequently validate against these types (rather than checking just that the input is text) . This in turn would result in something like map_html_path and plot_html_path output types that are effectively just logically distinct versions of PrecomputedHTMLWidgetData underneath. Then create_map_widget_single_view would only accept map_html_path as input. That seems like a nice way to line up inputs of create_widget tasks with the outputs of draw_ tasks. Though I feel like this runs the risk of quickly getting messy/out of hand as new tasks / features are added.