NRLMMD-GEOIPS / geoips

Main Geolocated Information Processing System code base with basic functionality enabled.
https://nrlmmd-geoips.github.io/geoips/
Other
13 stars 10 forks source link

Discuss how we want to support legacy procflow calls with new CLI #598

Closed evrose54 closed 1 week ago

evrose54 commented 1 month ago

Requested Update

Description

All GeoIPS / supporting GeoIPS Plugin Packages use some combination of run_procflow ... --procflow <procflow_name> or data_fusion_procflow ... --procflow data_fusion to produce their outputs. With the new updates of #455, we have changed this functionality with geiops run <procflow_name>. We've added code to support the aforementioned legacy calls, but it's more of a workaround than anything else.

We should discuss exactly how we want to support normal geoips run calls and legacy procflow calls for the future, until we have the order_based procflow functional. There are a few options @jsolbrig and I have discussed for how to handle this (handled in a separate PR from #455 as this will require a decent amount of refactoring):

  1. Keep legacy procflow calls as is and don't change their entry points (worst solution)
  2. Keep the CLI workaround as is and report the appropriate errors if encountered (not optimal)
  3. Support —procflow in geoips run calls. If provided:
    • Get the procflow and call the associated GeoipsRun<procflow_name> command class.
    • add known arguments to geoips run: —procflow (maybe others)
    • add additional subcommands as options if specified
    • @jsolbrig if you could expand on this portion I'd appreciate it. Still a bit fuzzy on this one
  4. Another option could be:
    • Just check if third option is a defined procflow (ie. geoips run config_based / data_fusion / single_source
    • parse it out and use the associated command
    • otherwise run order_based procflow when supported.

Background and Motivation

This came from a conversation about #455.

Alternative Solutions

Leave the code as is (that's what we're doing now but we need to address this eventually).

Code to demonstrate issue

jsolbrig commented 1 month ago

Good issue. Thank you, @evrose54.

Here is the original conversation regarding this issue, though some took place in an actual conversation.

For option 3 above, we could add --procflow to the top-level parser, then use parser.parse_known_args() to collect it before continuing to add subparsers. I think this would probably need to be done in GeoipsCLI.__init__().

Another option that occurred to me is to separate geoips run and another subcommand, geoips run-legacy where geoips run would call the order-based procflow and geoips run-legacy would allow calling the other procflows.

jsolbrig commented 1 week ago

We have a working option for this now, but I think we just want to discuss whether the CLI flags for this are correct.

jsolbrig commented 1 week ago

This was handled in #465. Closing. Will discuss if current solution is appropriate when going over the CLI next week.