Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jan 3, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
93feb7e
[#5960] fix(CLI): Add register and link commands to CLI for model
Abyss-lord Jan 2, 2025
ce05d1b
[#5960] fix(CLI): Add register and link commands to CLI for model
Abyss-lord Jan 2, 2025
a0fe9a2
[#5960] fix(CLI): Add register and link commands to CLI for model
Abyss-lord Jan 3, 2025
30ed52b
[#5960] fix(CLI): Add register and link commands to CLI for model
Abyss-lord Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion clients/cli/src/main/java/org/apache/gravitino/cli/ErrorMessages.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public class ErrorMessages {
public static final String MISSING_USER = "Missing --user option.";
public static final String MISSING_ROLE = "Missing --role option.";
public static final String MISSING_TAG = "Missing --tag option.";
public static final String MISSING_URI = "Missing --uri option.";
public static final String METALAKE_EXISTS = "Metalake already exists.";
public static final String CATALOG_EXISTS = "Catalog already exists.";
public static final String SCHEMA_EXISTS = "Schema already exists.";
Expand All @@ -51,13 +52,13 @@ public class ErrorMessages {
public static final String COLUMN_EXISTS = "Column already exists.";
public static final String UNKNOWN_TOPIC = "Unknown topic.";
public static final String TOPIC_EXISTS = "Topic already exists.";
public static final String MODEL_EXISTS = "Model already exists.";
public static final String UNKNOWN_FILESET = "Unknown fileset.";
public static final String FILESET_EXISTS = "Fileset already exists.";
public static final String TAG_EMPTY = "Error: Must configure --tag option.";
public static final String UNKNOWN_ROLE = "Unknown role.";
public static final String ROLE_EXISTS = "Role already exists.";
public static final String TABLE_EXISTS = "Table already exists.";
public static final String MODEL_EXISTS = "Model already exists.";
public static final String INVALID_SET_COMMAND =
"Unsupported combination of options either use --name, --user, --group or --property and --value.";
public static final String INVALID_REMOVE_COMMAND =
Expand Down
34 changes: 34 additions & 0 deletions clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1192,6 +1192,40 @@ private void handleModelCommand() {
}
break;

case CommandActions.CREATE:
String createComment = line.getOptionValue(GravitinoOptions.COMMENT);
String[] createProperties = line.getOptionValues(GravitinoOptions.PROPERTIES);
Map<String, String> createPropertyMap = new Properties().parse(createProperties);
newCreateModel(
url, ignore, metalake, catalog, schema, model, createComment, createPropertyMap)
.handle();
break;

case CommandActions.UPDATE:
String[] alias = line.getOptionValues(GravitinoOptions.ALIAS);
String uri = line.getOptionValue(GravitinoOptions.URI);
if (uri == null) {
Abyss-lord marked this conversation as resolved.
Show resolved Hide resolved
System.err.println(ErrorMessages.MISSING_URI);
Main.exit(-1);
}

String linkComment = line.getOptionValue(GravitinoOptions.COMMENT);
String[] linkProperties = line.getOptionValues(CommandActions.PROPERTIES);
Map<String, String> linkPropertityMap = new Properties().parse(linkProperties);
newLinkModel(
url,
ignore,
metalake,
catalog,
schema,
model,
uri,
alias,
linkComment,
linkPropertityMap)
.handle();
break;

default:
System.err.println(ErrorMessages.UNSUPPORTED_ACTION);
break;
Expand Down
6 changes: 6 additions & 0 deletions clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ public class GravitinoOptions {
public static final String ALL = "all";
public static final String ENABLE = "enable";
public static final String DISABLE = "disable";
public static final String ALIAS = "alias";
public static final String URI = "uri";

/**
* Builds and returns the CLI options for Gravitino.
Expand Down Expand Up @@ -109,6 +111,10 @@ public Options options() {
options.addOption(createArgOption(COLUMNFILE, "CSV file describing columns"));
options.addOption(createSimpleOption(null, ALL, "all operation for --enable"));

// model options
options.addOption(createArgOption(null, URI, "model version artifact"));
options.addOption(createArgsOption(null, ALIAS, "model aliases"));

// Options that support multiple values
options.addOption(createArgsOption("p", PROPERTIES, "property name/value pairs"));
options.addOption(createArgsOption("t", TAG, "tag name"));
Expand Down
29 changes: 29 additions & 0 deletions clients/cli/src/main/java/org/apache/gravitino/cli/TestableCommandLine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.apache.gravitino.cli.commands.GrantPrivilegesToRole;
import org.apache.gravitino.cli.commands.GroupAudit;
import org.apache.gravitino.cli.commands.GroupDetails;
import org.apache.gravitino.cli.commands.LinkModel;
import org.apache.gravitino.cli.commands.ListAllTags;
import org.apache.gravitino.cli.commands.ListCatalogProperties;
import org.apache.gravitino.cli.commands.ListCatalogs;
Expand Down Expand Up @@ -83,6 +84,7 @@
import org.apache.gravitino.cli.commands.ModelAudit;
import org.apache.gravitino.cli.commands.ModelDetails;
import org.apache.gravitino.cli.commands.OwnerDetails;
import org.apache.gravitino.cli.commands.RegisterModel;
import org.apache.gravitino.cli.commands.RemoveAllTags;
import org.apache.gravitino.cli.commands.RemoveCatalogProperty;
import org.apache.gravitino.cli.commands.RemoveFilesetProperty;
Expand Down Expand Up @@ -925,4 +927,31 @@ protected ModelDetails newModelDetails(
String url, boolean ignore, String metalake, String catalog, String schema, String model) {
return new ModelDetails(url, ignore, metalake, catalog, schema, model);
}

protected RegisterModel newCreateModel(
String url,
boolean ignore,
String metalake,
String catalog,
String schema,
String model,
String comment,
Map<String, String> properties) {
return new RegisterModel(url, ignore, metalake, catalog, schema, model, comment, properties);
}

protected LinkModel newLinkModel(
String url,
boolean ignore,
String metalake,
String catalog,
String schema,
String model,
String uri,
String[] alias,
String comment,
Map<String, String> properties) {
return new LinkModel(
url, ignore, metalake, catalog, schema, model, uri, alias, comment, properties);
}
}
106 changes: 106 additions & 0 deletions clients/cli/src/main/java/org/apache/gravitino/cli/commands/LinkModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.gravitino.cli.commands;

/** Link a new model version to the registered model. */
import java.util.Arrays;
import java.util.Map;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.cli.ErrorMessages;
import org.apache.gravitino.client.GravitinoClient;
import org.apache.gravitino.exceptions.ModelVersionAliasesAlreadyExistException;
import org.apache.gravitino.exceptions.NoSuchCatalogException;
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
import org.apache.gravitino.exceptions.NoSuchModelException;
import org.apache.gravitino.exceptions.NoSuchSchemaException;
import org.apache.gravitino.model.ModelCatalog;

public class LinkModel extends Command {
protected final String metalake;
protected final String catalog;
protected final String schema;
protected final String model;
protected final String uri;
protected final String[] alias;
protected final String comment;
protected final Map<String, String> properties;

/**
* Link a new model version to the registered model.
*
* @param url The URL of the Gravitino server.
* @param ignoreVersions If true don't check the client/server versions match.
* @param metalake The name of the metalake.
* @param catalog The name of the catalog.
* @param schema The name of schema.
* @param model The name of model.
* @param uri The URI of the model version artifact.
* @param alias The aliases of the model version.
* @param comment The comment of the model version.
* @param properties The properties of the model version.
*/
public LinkModel(
String url,
boolean ignoreVersions,
String metalake,
String catalog,
String schema,
String model,
String uri,
String[] alias,
String comment,
Map<String, String> properties) {
super(url, ignoreVersions);
this.metalake = metalake;
this.catalog = catalog;
this.schema = schema;
this.model = model;
this.uri = uri;
this.alias = alias;
this.comment = comment;
this.properties = properties;
}

/** Link a new model version to the registered model. */
@Override
public void handle() {
NameIdentifier name = NameIdentifier.of(schema, model);

try {
GravitinoClient client = buildClient(metalake);
ModelCatalog modelCatalog = client.loadCatalog(catalog).asModelCatalog();
modelCatalog.linkModelVersion(name, uri, alias, comment, properties);
} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
} catch (NoSuchCatalogException err) {
exitWithError(ErrorMessages.UNKNOWN_CATALOG);
} catch (NoSuchSchemaException err) {
exitWithError(ErrorMessages.UNKNOWN_SCHEMA);
} catch (NoSuchModelException err) {
exitWithError(ErrorMessages.UNKNOWN_MODEL);
} catch (ModelVersionAliasesAlreadyExistException err) {
exitWithError(Arrays.toString(alias) + " already exist.");
} catch (Exception err) {
exitWithError(err.getMessage());
}

System.out.println(
"Linked model " + model + " to " + uri + " with aliases " + Arrays.toString(alias));
}
}
103 changes: 103 additions & 0 deletions clients/cli/src/main/java/org/apache/gravitino/cli/commands/RegisterModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.apache.gravitino.cli.commands;

import java.util.Map;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.cli.ErrorMessages;
import org.apache.gravitino.cli.Main;
import org.apache.gravitino.client.GravitinoClient;
import org.apache.gravitino.exceptions.ModelAlreadyExistsException;
import org.apache.gravitino.exceptions.NoSuchCatalogException;
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
import org.apache.gravitino.exceptions.NoSuchSchemaException;
import org.apache.gravitino.model.Model;
import org.apache.gravitino.model.ModelCatalog;

/** Register a model in the catalog */
public class RegisterModel extends Command {

protected final String metalake;
protected final String catalog;
protected final String schema;
protected final String model;
protected final String comment;
protected final Map<String, String> properties;

/**
* Register a model in the catalog
*
* @param url The URL of the Gravitino server.
* @param ignoreVersions If true don't check the client/server versions match.
* @param metalake The name of the metalake.
* @param catalog The name of the catalog.
* @param schema The name of schema.
* @param model The name of model.
* @param comment The comment of the model version.
* @param properties The properties of the model version.
*/
public RegisterModel(
String url,
boolean ignoreVersions,
String metalake,
String catalog,
String schema,
String model,
String comment,
Map<String, String> properties) {
super(url, ignoreVersions);
this.metalake = metalake;
this.catalog = catalog;
this.schema = schema;
this.model = model;
this.comment = comment;
this.properties = properties;
}

/** Register a model in the catalog */
@Override
public void handle() {
NameIdentifier name = NameIdentifier.of(schema, model);
Abyss-lord marked this conversation as resolved.
Show resolved Hide resolved
Model registeredModel = null;

try {
GravitinoClient client = buildClient(metalake);
ModelCatalog modelCatalog = client.loadCatalog(catalog).asModelCatalog();
registeredModel = modelCatalog.registerModel(name, comment, properties);
} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
} catch (NoSuchCatalogException err) {
exitWithError(ErrorMessages.UNKNOWN_CATALOG);
} catch (NoSuchSchemaException err) {
exitWithError(ErrorMessages.UNKNOWN_SCHEMA);
} catch (ModelAlreadyExistsException err) {
exitWithError(ErrorMessages.MODEL_EXISTS);
} catch (Exception err) {
Abyss-lord marked this conversation as resolved.
Show resolved Hide resolved
exitWithError(err.getMessage());
}

if (registeredModel != null) {
System.out.println("Successful register " + registeredModel.name() + ".");
} else {
System.err.println("Failed to register model: " + model + ".");
Abyss-lord marked this conversation as resolved.
Show resolved Hide resolved
Main.exit(-1);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@justinmclean justinmclean Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
Loading
Loading