RSDK-9284 - automate CLI flag parsing #4581

stuqdog · 2024-11-22T19:28:43Z

Note: everything below the line was written when this PR was a POC and may be out of date.

Adds typed argument structs to all CLI commands, creates a simple interface for developers to do the same for new commands.

Testing: confirmed locally that all existing flag types (int, uint, string, string slice, bool, duration, path) can be parsed and set correctly. Confirmed values in several commands.

Note to reviewers: this is currently a POC and definitely not ready for prime time. Before I go ahead and make changes to all existing methods, I wanted to get buy-in from folks on this as an approach. Once we have agreement on the shape of this implementation, I'll do the (verbose, but mechanical) work of changing existing methods which should hopefully be trivial to review despite having an estimated large loc diff.

The nice thing about this approach is it provides safe, easy, typeful access to flag data while requiring minimal change in how developers create actions (they have to define a struct with their flag fields and the Action field is now populated slightly differently), but there should be an easily replicable pattern in the code base such that this is not harmful.

A notable shortcoming of this approach is that the names of the fields in the struct must match (or fuzzy match, taking account for differences in snake/camel/kebab case) the names of the flag. Since a developer is defining the struct, we don't currently have a way to enforce that things are correct. I do think it would be possible using reflection to have some sort of assert that the flags and the fields of the struct fuzzy match, but I fear this will require some decent refactoring and is more of a "programmatic enforcement" issue rather than a "automate flag parsing" issue and so should be done in a future PR.

One other (minor) restriction: the fields on the struct must be public. The reflection library can't access them if they're private, leading to a runtime error.

One other thing to note: I don't think we can get away with not requiring a CLI.context in the structful methods that we're asking users to now define, as certain existing methods (DownloadModuleAction, e.g.) use fields on the ctx within the method. This means that if a user wants to access flags the old way, we can't stop them. Again, this might be resolvable but probably should go in a separate ticket.

stuqdog · 2024-11-22T19:31:47Z

cli/app.go

+	camelFormattedName = matchAllCap.ReplaceAllString(camelFormattedName, "${1}-${2}")
+	camelFormattedName = strings.ToLower(camelFormattedName)
+
+	return ctx.Value(camelFormattedName)


It seems like some guardrails might be nice here, but I'm not sure that we can say at compile time what's safe and what's not. Even if an argument is non-optional, I expect there are cases where a nil value is normal and expected.

stuqdog · 2024-11-22T19:32:37Z

cli/app.go

+type foo struct {
+	FooFoo string
+	Bar    int
+}
+
+func doFoo(foo foo, ctx *cli.Context) error {
+	fmt.Printf("FooFoo is %s and Bar is %v.", foo.FooFoo, foo.Bar)
+	return nil
+}


This will get deleted in the final project obviously, but I wanted to show an example of what new development would look like. Also, it highlights how we successfully set a field FooFoo with a flag of foo-foo

stuqdog · 2024-11-22T19:59:48Z

fyi @dgottlieb since we discussed this idea a bit during the scope doc process.

benjirewis · 2024-11-22T22:01:26Z

cc @zaporter-work

zaporter-work · 2024-11-25T05:41:11Z

This seems like a good improvement -- I like the flags object (hopefully people will catch name-mismatches manually). However, If you're interested in other options, I've enjoyed this pattern for some of my personal projects. It takes a bit of naming-awareness, but it is fairly simple. This uses urfavecli/v3 instead of v2 (though it might be compatible with v2):
main.go:

func main() {
	cmd := &cli.Command{
		Name: "sudoku",
		Commands: []*cli.Command{
			createStatsCli(),
			createDataCli(),
			createGraphCli(),
			createEvaluateCli(),
			createExperimentCli(),
			lambda.CreateLambdaCli(),
		},
	}
	if err := cmd.Run(context.Background(), os.Args); err != nil {
		log.Fatalln("error:", err)
	}
}

example stat.go

func createStatsCli() *cli.Command {
	graphPath := ""
        printUnfinishedSteps := false
	action := func(_ context.Context, _ *cli.Command) error {
                // SOME VERY SHORT FUNCTION
                // (that uses the vars in the parent scope)
		graph, err := LoadGraphFromFile(graphPath)
		if err != nil {
			return err
		}

		var pStats []ProblemStats
		for _, p := range graph.Problems {
			pStats = append(pStats, p.GenStats(graph))
		}
		PrintStatsInfo(pStats, printUnfinishedSteps)
		return nil
	}

	return &cli.Command{
		Name: "stats",
		Arguments: []cli.Argument{
			&cli.StringArg{
				Name:        "graph",
				Destination: &graphPath,
			},
		},
		Flags: []cli.Flag{
			&cli.BoolFlag{
				Name:        "print-unfinished-steps",
				Destination: &printUnfinishedSteps,
			},
		},
		Action: action,
                // can support nested Commands via
               // Commands: []*cli.Command{moreCreateInvocations()}
	}
}

This has the benefit of not using runtime reflection -- though it does scatter a lot of the cli tree out into different functions instead of one giant object. I like it for personal projects because the Destination: &var means I don't have to worry about any magic strings in contexts. (it also makes it easy to use Arguments instead of just Flags)

No pressure to use this though! Just wanted to throw something out there to consider while you restructure the flag parsing.

benjirewis

Thanks for thinking about this + putting a PoC together @stuqdog! I really like how this method does not massively change how we're adding new pieces to the highest level cli.App struct. I do have three issues with it:

You are storing values on contexts
a. I can totally get over this one, but it is generally an anti-pattern in Go in the sense that it should be a "last-resort" strategy (this might be a "last-resort" case)
You are using runtime reflection
a. Your reflection code (getValFromContext) will run for each command at each invocation. Putting reflection in "hot paths" is usually a bad idea because reflection is relatively slow
Your fuzzy searching allows multiple ways of specifying a flag
a. Most CLIs I have interacted with only allow one or two ways of specifying a flag (usually -c and -count or something similar) and are pretty stringent about using hyphens between words instead of underscores. I think users will be surprised by the flexibility here especially if it is undocumented

I like the idea of @zaporter-work's solution, but it looks like we are expecting developers to use the Destination pattern (which is a good one!) in their create*CLI functions. If they forget to do that (which I think they will, honestly,) then all we've done is, as Zack mentioned, obfuscate the definition of the top-level cli.App.

I realize I am only criticizing and not offering other solutions 😮‍💨 ; let me take a stab at something locally and get back to you guys. If we really want to get Viam developers to add ActionFuncs in a more consistent manner, I think we'll have to design a new API for adding a Command, and a custom struct that wraps a *cli.App.

stuqdog · 2024-11-26T16:26:31Z

Thanks for thinking about this + putting a PoC together @stuqdog! I really like how this method does not massively change how we're adding new pieces to the highest level cli.App struct. I do have three issues with it:
1. You are storing values on contexts
   a. I can totally get over this one, but it is generally an anti-pattern in Go in the sense that it     should be a "last-resort" strategy (this might be a "last-resort" case)

2. You are using runtime reflection
   a. Your reflection code (`getValFromContext`) will run for each command at each invocation. Putting reflection in "hot paths" is usually a bad idea because reflection is relatively slow

3. Your fuzzy searching allows multiple ways of specifying a flag
   a. Most CLIs I have interacted with only allow one or two ways of specifying a flag (usually `-c` and `-count` or something similar) and are pretty stringent about using hyphens between words instead of underscores. I think users will be surprised by the flexibility here especially if it is undocumented
I like the idea of @zaporter-work's solution, but it looks like we are expecting developers to use the Destination pattern (which is a good one!) in their create*CLI functions. If they forget to do that (which I think they will, honestly,) then all we've done is, as Zack mentioned, obfuscate the definition of the top-level cli.App.

I realize I am only criticizing and not offering other solutions 😮‍💨 ; let me take a stab at something locally and get back to you guys. If we really want to get Viam developers to add ActionFuncs in a more consistent manner, I think we'll have to design a new API for adding a Command, and a custom struct that wraps a *cli.App.

Hey! I think there's some slight misunderstanding specifically around points 1 and 3, messaged you offline to chat about it :)

stuqdog · 2024-11-26T19:23:31Z

If they forget to do that (which I think they will, honestly,) then all we've done is, as Zack mentioned, obfuscate the definition of the top-level cli.App.

Yeah I like @zaporter-work's suggestion a lot but I do worry it expects too much of developers in terms of remembering the right way to do things.

@benjirewis and I are speaking this afternoon and will come up with some thoughts on how to proceed from there!

benjirewis

@stuqdog and I spoke offline, and he assuaged all three of my previous concerns. I cannot come up with some clever way to do this without any runtime reflection, but we both agree that the performance hit of using reflection here is fine given the usage of this code in CLI (user is almost always entering one command and waiting for a response.) We did want to create a ticket for a subsequent PR that wraps the cli.App in a custom ViamCLIApp struct that introduces a bit more type safety around adding new commands.

I do have a couple nits and one question about your handling of types beyond string/optional types.

benjirewis · 2024-11-26T20:17:33Z

cli/app.go

@@ -125,6 +127,11 @@ const (
 	cpFlagPreserve  = "preserve"
 )

+var (
+	matchFirstCap = regexp.MustCompile("(.)([A-Z][a-z]+)")
+	matchAllCap   = regexp.MustCompile("([a-z0-9])([A-Z])")


[nit] Could you add comments above each of these regexes giving an example of a pattern they would match? I find that to be extremely helpful when reading through code and seeing (well-named but otherwise) random regular expressions.

Ooh yeah, good call. Will add comments!

Upon looking I have concluded that matchFirstCap is a bit extraneous. That's what I get for copying someone else's regex here! Removed it, added an explanatory comment for matchAllCap.

benjirewis · 2024-11-26T20:18:39Z

cli/app.go

@@ -224,6 +231,49 @@ var dataTagByFilterFlags = append([]cli.Flag{
 },
 	commonFilterFlags...)

+func getValFromContext(name string, ctx *cli.Context) interface{} {


Suggested change

func getValFromContext(name string, ctx *cli.Context) interface{} {

func getValFromContext(name string, ctx *cli.Context) any {

[nit] Prefer any to interface{} post Golang 1.18 (here and elsewhere.)

🙏 thanks for the tip, will replace interface{} with any!

cli/app.go

benjirewis · 2024-11-26T20:32:58Z

cli/app.go

+
+type foo struct {
+	FooFoo string
+	Bar    int


I'm surprised you can have int fields here. I thought the value stored on the cli.Context would be a string. How do you not get a runtime error on L260 above when you try to set the Bar int value to a string? Similarly, if a user does not specify -bar flag, would we not try to set the Bar int value to nil?

Good question! So the value stored on the cli.Context is of any type, we can see this being used in the existing code here e.g., where we're getting a uint out of the context. What this is doing is grabbing out the requested value as an interface, and then using reflection to determine the expected type and plug it into the struct.

If we don't pass a flag then we get nil, which is parsed as the zero value. So, Bar becomes 0 and foo becomes "". Which, now that I think about it, is probably not ideal. Probably we should have the struct setup such that optional values are pointers, and we do a if val != nil check when setting them in createCommandWithT.

For non-optional types, the existing process of marking a flag as Required will still be sufficient to enforce its use.

Gotcha! Thanks for the explanation; makes sense.

Probably we should have the struct setup such that optional values are pointers, and we do a if val != nil check when setting them in createCommandWithT.

I wonder if Bar *int, at least, is an important field to use a pointer with to distinguish between the user passing 0 and passing nothing.

benjirewis

Pretty much LGTM! Curious whether you'll make the *Flags structs have pointer types or just string, int, etc.

benjirewis · 2024-12-02T17:08:26Z

cli/app.go

+
+type foo struct {
+	FooFoo string
+	Bar    int


Gotcha! Thanks for the explanation; makes sense.

Probably we should have the struct setup such that optional values are pointers, and we do a if val != nil check when setting them in createCommandWithT.

I wonder if Bar *int, at least, is an important field to use a pointer with to distinguish between the user passing 0 and passing nothing.

njooma

This looks good! My only question is that I know we're looking at alternative libraries for CLI tooling that offers more features/flexibility, but that is faaaaar outside the scope of this current PR.

Can this pattern be migrated over to the new library should we ever move to that ourselves?

stuqdog · 2024-12-04T14:50:50Z

This looks good! My only question is that I know we're looking at alternative libraries for CLI tooling that offers more features/flexibility, but that is faaaaar outside the scope of this current PR.

Can this pattern be migrated over to the new library should we ever move to that ourselves?

I suspect so, yes! Probably some massaging would be necessary depending on the library but I don't think this pattern is terribly complicated or unique to this library.

I do also think that we have ways of achieving what we want within the confines of urfave as well, so if we're fine with a bit of massaging upfront I don't know that switching libraries is particularly important or urgent.

stuqdog · 2024-12-06T18:59:41Z

cli/app.go

+								&cli.StringSliceFlag{
+									Name: dataFlagBboxLabels,
+									Usage: "bbox labels filter. " +
+										"accepts string labels corresponding to bounding boxes within images",


(flyby) this flag was missing here but is accepted elsewhere where we want to construct a filter. Adding it here makes it easier to have shared arg groups for various Data methods, avoiding the need for repetitive struct definitions. Spoke with @dmhilly who confirmed this was acceptable to add.

stuqdog · 2024-12-06T19:00:32Z

cli/app.go

 									},
-									Action: DataAddToDatasetByFilter,
+										commonFilterFlags...),


(flyby) we were using all the commonFilterFlags here but rearticulating them with identical descriptions. Removed, moved to appending the commonFilterFlags.

stuqdog · 2024-12-06T19:01:22Z

cli/app.go

+										&cli.StringFlag{
+											Name:     mlTrainingFlagURL,
+											Usage:    "url of Github repository associated with the training scripts",
+											Required: false,
+										},


(flyby) added this flag, which was being searched for in the withUpload method but wasn't settable.
@etai-shuchatowitz @tahiyasalam I copied the flag details from other commands, if you have any issues with the shape of the flag here, please let me know!

great, thank you!

stuqdog · 2024-12-06T19:02:45Z

cli/app.go

+										&cli.StringFlag{
+											Name:     generalFlagOrgID,
+											Usage:    "org ID to train and save ML model in",
+											Required: true,
+										},


(flyby) added this flag, which was being searched for in the withUpload method but wasn't settable.
@etai-shuchatowitz @tahiyasalam I copied the flag details from other commands, if you have any issues with the shape of the flag here (usage text, etc.), please let me know!

Ah looking through this now, I am realizing there's actually a bug in our existing withUpload method.

The trainFlagModelOrgID here and here should actually in fact be the generalFlagOrgID, where the generalFlagOrgID is the "org ID to save the custom training script in". I hate to sneak in too many changes here, so I'm also happy with removing this from this PR & taking on as a follow up for my team or merging in that fix first for you to pull down into your branch. Let me know!

Hey, I think that's not a problem to do here! I just made the change, let me know if it still looks off :)

Looks good to me! :)

benjirewis

Didn't review line-by-line, but the general concept seems good to me. Thanks for this big overhaul @stuqdog !

benjirewis · 2024-12-09T19:14:13Z

cli/app.go

+// TODO(RSDK-9447) - We don't support pointers in this. The problem is that when getting a value
+// from a context for a supported flag, the context will default to populating with the zero value.
+// When getting a value from the context, though, we currently have no way of know if that's going
+// to a concrete value, going to a pointer and should be a nil value, or going to a pointer but should
+// be a pointer to that default value.


Sounds good; I'm a little confused about the explanation here but totally fine revisiting this as part of a separate ticket.

stuqdog · 2024-12-11T19:36:31Z

cli/app.go

-							Action: OrganizationDisableBillingServiceAction,
+							Action: createCommandWithT[organizationDisableBillingServiceArgs](OrganizationDisableBillingServiceAction),
 						},
 						{
 							Name:  "update",
 							Usage: "update the billing service update for an organization",
 							Flags: []cli.Flag{
+								&cli.StringFlag{
+									Name:     generalFlagOrgID,
+									Required: true,
+									Usage:    "the org to update the billing service for",
+								},
 								&cli.StringFlag{
 									Name:     organizationBillingAddress,
 									Required: true,
 									Usage:    "the stringified address that follows the pattern: line1, line2 (optional), city, state, zipcode",
 								},
 							},
-							Action: UpdateBillingServiceAction,
+							Action: createCommandWithT[updateBillingServiceArgs](UpdateBillingServiceAction),


fyi adding flag back as per offline convo @RoxyFarhad

tahiyasalam

ML training changes look good to me. I commented on the things that I verified, but let me know if you need me to look at anything else.

tahiyasalam · 2024-12-11T19:49:46Z

cli/app.go

 										&cli.StringFlag{
 											Name:     mlTrainingFlagPath,
 											Usage:    "path to ML training scripts for upload",
 											Required: true,
 										},
+										&cli.StringFlag{


Makes sense to me, thank you!

🙏 thanks!

stuqdog requested review from njooma and benjirewis November 22, 2024 19:28

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 22, 2024

stuqdog commented Nov 22, 2024

View reviewed changes

stuqdog marked this pull request as draft November 22, 2024 20:29

stuqdog marked this pull request as ready for review November 22, 2024 20:46

benjirewis reviewed Nov 26, 2024

View reviewed changes

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 2, 2024

stuqdog requested a review from benjirewis December 2, 2024 15:51

benjirewis approved these changes Dec 2, 2024

View reviewed changes

njooma approved these changes Dec 3, 2024

View reviewed changes

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 6, 2024

stuqdog force-pushed the automate-cli-flag-parsing branch from e6a4e94 to b709165 Compare December 6, 2024 18:56

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 6, 2024

stuqdog commented Dec 6, 2024

View reviewed changes

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 6, 2024

stuqdog added 13 commits December 6, 2024 16:23

poc

de8fc04

no intermediate variable name

22d7473

ctx should always be first arg

0f6bf67

pr comments

ddd759a

move to struct-based flag approach

4157e0a

test fixes

2b2d3fd

lint fixes

f972edd

remove CR comments to self

3056528

empty arg name consistency

a7e7b52

add todo, move checkUpdateAction to using global args

b216c6c

fix failing test

7e21ab6

update get and set support email

3972279

update getbillingconfig

ed6c00c

stuqdog force-pushed the automate-cli-flag-parsing branch from 415fb64 to ed6c00c Compare December 6, 2024 21:25

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 6, 2024

fix mltrainingwithupload

337a851

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 6, 2024

benjirewis approved these changes Dec 9, 2024

View reviewed changes

njooma approved these changes Dec 11, 2024

View reviewed changes

update new funcs

f847e20

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 11, 2024

stuqdog commented Dec 11, 2024

View reviewed changes

tahiyasalam reviewed Dec 11, 2024

View reviewed changes

stuqdog merged commit 5720618 into viamrobotics:main Dec 11, 2024
16 checks passed

stuqdog deleted the automate-cli-flag-parsing branch December 12, 2024 14:13

	func getValFromContext(name string, ctx *cli.Context) interface{} {
	func getValFromContext(name string, ctx *cli.Context) any {

RSDK-9284 - automate CLI flag parsing #4581

RSDK-9284 - automate CLI flag parsing #4581

Uh oh!

Conversation

stuqdog commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuqdog commented Nov 22, 2024

Uh oh!

benjirewis commented Nov 22, 2024

Uh oh!

zaporter-work commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjirewis left a comment

Choose a reason for hiding this comment

Uh oh!

stuqdog commented Nov 26, 2024

Uh oh!

stuqdog commented Nov 26, 2024

Uh oh!

benjirewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuqdog Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjirewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njooma left a comment

Choose a reason for hiding this comment

Uh oh!

stuqdog commented Dec 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjirewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuqdog commented Nov 22, 2024 •

edited

Loading

zaporter-work commented Nov 25, 2024 •

edited

Loading

stuqdog Dec 2, 2024 •

edited

Loading