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.

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

REVAI-3854: SuperApi features #59

Merged
merged 6 commits into from
Dec 16, 2023
Merged

REVAI-3854: SuperApi features #59

merged 6 commits into from
Dec 16, 2023

Conversation

kirillatrev
Copy link
Collaborator

REVAI-3854: Initial draft

@kirillatrev kirillatrev requested a review from a team as a code owner December 13, 2023 22:35
@kirillatrev kirillatrev marked this pull request as draft December 13, 2023 22:42
@kirillatrev kirillatrev marked this pull request as ready for review December 14, 2023 23:21
@kirillatrev kirillatrev changed the title REVAI-3854: Initial draft REVAI-3854: SuperApi features Dec 15, 2023

import com.google.gson.annotations.SerializedName;

/** Model type. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like can be more verbose


@Override
public String toString() {
return "{" + "model='" + model + '\'' + '}';
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the purpose of having it in this format? why can't consumer of this type format it as needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know what the intention was, but I follow the same pattern. JobOptions class at the top level calls .toString() on every member and expects this output.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, really. at last for some nested models we have this formatting.
@amikofalvy by any chance do you know what was the purpose?

this.summarizationOptions = summarizationOptions;
}

public TranslationOptions getTranslationOptions() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think here should be a comment

private String prompt;

/** Standard or Premium AI backend. * */
@SerializedName("model")
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure we can have any other model type, but we can say something like summarization model and then @see NlpModel. then we can extend the NlpModel as we need without other changes


import com.google.gson.annotations.SerializedName;

/** Summarization formatting options. * */
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a purpose for * */? I see it everywhere in this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is how autoformatted puts it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did not find anything relevant in Google. Also, I don't see it like this in other files. Even in some of your files there is a single star only

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay. Thanks auto formatter! :-)
I'll spend some more time on this.

@kirillatrev kirillatrev merged commit 4a97be6 into develop Dec 16, 2023
6 checks passed
@kirillatrev kirillatrev deleted the feature/REVAI-3854 branch December 16, 2023 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants