[#5960] fix(CLI): Add register and link commands to CLI for model #6066

Abyss-lord · 2025-01-02T09:31:00Z

What changes were proposed in this pull request?

Add register and link commands to CLI for model

register a model：model create
link a model：model update <—uri uri> [--alias aliaA aliaB]

meantime, add two options

—uri ：The URI of the model version artifact.
—alias ：The aliases of the model version.

The documentation will be updated after #6047 merge and I will create a new issue.

Why are the changes needed?

Fix: #5960

Does this PR introduce any user-facing change?

NO

How was this patch tested?

register test

# register a model
gcli model create -m demo_metalake --name Hive_catalog.default.model
# register a model with comment
gcli model create -m demo_metalake --name Hive_catalog.default.model --comment comment
# register a model with properties
gcli model create -m demo_metalake --name Hive_catalog.default.model --properties key1=val1 key2=val2
# register a model with properties and comment
gcli model create -m demo_metalake --name Hive_catalog.default.model --properties key1=val1 klinkey2=val2 --comment comment

link test

# link a model 
gcli model update -m demo_metalake --name Hive_catalog.default.model --uri file:///tmp/file
# link a model with alias
gcli model update -m demo_metalake --name Hive_catalog.default.model --uri file:///tmp/file  --alias aliasA aliasB
# link a model with all component
gcli model update -m demo_metalake --name Hive_catalog.default.model --uri file:///tmp/file  --alias aliasA aliasB --comment comment --properties key1=val1 key2=val2
# link a model without uri
gcli model update -m demo_metalake --name Hive_catalog.default.model

Abyss-lord · 2025-01-02T09:31:21Z

H @justinmclean @tengqm , could you please review this PR when you have time? I’d really appreciate your feedback.

clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java

tengqm · 2025-01-02T12:24:36Z

clients/cli/src/main/java/org/apache/gravitino/cli/commands/RegisterModel.java

+      System.out.println("Successful register " + registeredModel.name() + ".");
+    } else {
+      System.err.println("Failed to register model: " + model + ".");
+    }


Two confusions here:

Do we have a protocol regarding where to output error/info messages?

Are we always printing something on the command line when some actions were successful?

I am thinking of a scenario where the command line tool is invoked from an upper
layer script. Getting this verbose message are not so "helpful" as we expect.

Yes errors do to standard output and the program should exit with -1. Scripts can check the exit code to see if something worked. Output like this is helpful to users, as if a command has no output, they don't know if it worked or not.
From https://clig.dev:

Return zero exit code on success, non-zero on failure.

Send output to stdout.

Send messaging to stderr.

Display output on success, but keep it brief.

In this case, I probably wouldn't mention the model name on success, but it is probably OK to do so.

I don't think "display output on success ..." is a common practice.

(venv) tmp $ rm q19.zip (venv) tmp $ rm q19.zip rm: q19.zip: No such file or directory

For commands like rm and others that were written 30 or so years ago, they had little output. Thinking about that has changed over those decades. I would suggest we follow the advice in that document. Here it is in full:
"Display output on success, but keep it brief. Traditionally, when nothing is wrong, UNIX commands display no output to the user. This makes sense when they’re being used in scripts, but can make commands appear to be hanging or broken when used by humans. For example, cp will not print anything, even if it takes a long time.
It’s rare that printing nothing at all is the best default behavior, but it’s usually best to err on the side of less."

You note that more modern commands (like git or the GitHub CLI) do output on success and follow this design.

It also suggests that if you want no output, you should add a --quiet flag to do that.

@justinmclean , Should we add the --quit option？

--quiet not --quit :-) There is no need to add it here, as it would impact many commands. We can add it in another issue/PR if we think it is needed.

I like the --quiet idea.
I am not a big fan of verbose outputs from commands. For example, when I'm doing gcli get something, I'm expecting an output which is the data I'm retrieving. I may go further down this road anticipating a --format option, with which I can dump the output as a JSON. Yes, we can do an --output to dump/redirect this data. But this is not script friendly.

For commands other than get or list, I'm not expecting data from the server.
I'm not expecting an output from the client side tool either (I admit that this is a personal taste and I don't want to argue over this).
The bizarre point is that ... the CLI now has two kinds of outputs.
Sometimes, it produces meaningful (business) data and prints them out
(think the get, list case), other times ... it is producing something verbose,
just for the purpose of producing something.

By the way, the cp command is a synchronous action. It will not return control to
you until it succeeds or fail with an error message. It is not a good example, though I get your points.

We already have an option for different formats (currently plain and table). However, we would unlikely add a JSON format as the REST interface already provides information in that format. But if there was demand for it or someone wanted to develop that feature, they could. Originally, the CLI prototype was a wrapper on the REST and provided plain text and JSON output, but it was decided to wrap the Java API instead.

Note it is also recommended that commands that take time to run produce output so the user knows they are doing something. Again, more modern commands like git will do this. We currently don't have any commands that take a long time to run.

clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java

clients/cli/src/main/java/org/apache/gravitino/cli/commands/LinkModel.java

clients/cli/src/main/java/org/apache/gravitino/cli/commands/RegisterModel.java

clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java

clients/cli/src/main/java/org/apache/gravitino/cli/commands/RegisterModel.java

tengqm · 2025-01-03T00:19:22Z

clients/cli/src/main/java/org/apache/gravitino/cli/commands/RegisterModel.java

+      System.out.println("Successful register " + registeredModel.name() + ".");
+    } else {
+      System.err.println("Failed to register model: " + model + ".");
+    }


I don't think "display output on success ..." is a common practice.

(venv) tmp $ rm q19.zip (venv) tmp $ rm q19.zip rm: q19.zip: No such file or directory

1. Add register and link commands to CLI for model. 2. Add two option for model --uri and --alias.

add test case.

Fix some error.

Abyss-lord · 2025-01-03T09:20:23Z

@justinmclean , I’ve finished updating the code. Please take a look at the PR again when you have time.

justinmclean

LGTM thanks again for your contribution.

…el (apache#6066) Add register and link commands to CLI for model - register a model：`model create` - link a model：`model update <—uri uri> [--alias aliaA aliaB]` meantime, add two options - `—uri` ：The URI of the model version artifact. - `—alias` ：The aliases of the model version. The documentation will be updated after apache#6047 merge and I will create a new issue. Fix: apache#5960 NO ```bash gcli model create -m demo_metalake --name Hive_catalog.default.model gcli model create -m demo_metalake --name Hive_catalog.default.model --comment comment gcli model create -m demo_metalake --name Hive_catalog.default.model --properties key1=val1 key2=val2 gcli model create -m demo_metalake --name Hive_catalog.default.model --properties key1=val1 klinkey2=val2 --comment comment ``` ```bash gcli model update -m demo_metalake --name Hive_catalog.default.model --uri file:///tmp/file gcli model update -m demo_metalake --name Hive_catalog.default.model --uri file:///tmp/file --alias aliasA aliasB gcli model update -m demo_metalake --name Hive_catalog.default.model --uri file:///tmp/file --alias aliasA aliasB --comment comment --properties key1=val1 key2=val2 gcli model update -m demo_metalake --name Hive_catalog.default.model ```

orenccl assigned Abyss-lord Jan 2, 2025

tengqm reviewed Jan 2, 2025

View reviewed changes

justinmclean reviewed Jan 2, 2025

View reviewed changes

clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java Outdated Show resolved Hide resolved

justinmclean reviewed Jan 2, 2025

View reviewed changes

clients/cli/src/main/java/org/apache/gravitino/cli/commands/LinkModel.java Outdated Show resolved Hide resolved

justinmclean reviewed Jan 2, 2025

View reviewed changes

clients/cli/src/main/java/org/apache/gravitino/cli/commands/RegisterModel.java Show resolved Hide resolved

justinmclean reviewed Jan 2, 2025

View reviewed changes

clients/cli/src/main/java/org/apache/gravitino/cli/commands/RegisterModel.java Show resolved Hide resolved

tengqm reviewed Jan 3, 2025

View reviewed changes

Abyss-lord force-pushed the model-command branch from 74f370e to 6bafa74 Compare January 3, 2025 02:32

Abyss-lord added 3 commits January 3, 2025 10:33

[apache#5960] fix(CLI): Add register and link commands to CLI for model

93feb7e

1. Add register and link commands to CLI for model. 2. Add two option for model --uri and --alias.

[apache#5960] fix(CLI): Add register and link commands to CLI for model

ce05d1b

add test case.

[apache#5960] fix(CLI): Add register and link commands to CLI for model

a0fe9a2

Fix some error.

Abyss-lord force-pushed the model-command branch from 6bafa74 to a0fe9a2 Compare January 3, 2025 02:33

Abyss-lord requested a review from justinmclean January 3, 2025 02:33

[apache#5960] fix(CLI): Add register and link commands to CLI for model

30ed52b

Fix some error.

justinmclean approved these changes Jan 3, 2025

View reviewed changes

justinmclean merged commit bfb8568 into apache:main Jan 3, 2025
25 checks passed

Abyss-lord deleted the model-command branch January 4, 2025 04:16

justinmclean mentioned this pull request Jan 5, 2025

[Subtask] Add create and delete CLI commands for Model #5963

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#5960] fix(CLI): Add register and link commands to CLI for model #6066

[#5960] fix(CLI): Add register and link commands to CLI for model #6066

Abyss-lord commented Jan 2, 2025 •

edited

Loading

Abyss-lord commented Jan 2, 2025

tengqm Jan 2, 2025

justinmclean Jan 2, 2025

tengqm Jan 3, 2025

justinmclean Jan 3, 2025

Abyss-lord Jan 3, 2025

justinmclean Jan 3, 2025

tengqm Jan 3, 2025

justinmclean Jan 3, 2025

justinmclean Jan 3, 2025 •

edited

Loading

tengqm Jan 3, 2025

Abyss-lord commented Jan 3, 2025

justinmclean left a comment

[#5960] fix(CLI): Add register and link commands to CLI for model #6066

[#5960] fix(CLI): Add register and link commands to CLI for model #6066

Conversation

Abyss-lord commented Jan 2, 2025 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

register test

link test

Abyss-lord commented Jan 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmclean Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Abyss-lord commented Jan 3, 2025

justinmclean left a comment

Choose a reason for hiding this comment

Abyss-lord commented Jan 2, 2025 •

edited

Loading

justinmclean Jan 3, 2025 •

edited

Loading