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

[#5965] subtask(web): ML model support for web UI #6025

Merged
merged 4 commits into from
Dec 30, 2024

Conversation

LauraXia123
Copy link
Collaborator

@LauraXia123 LauraXia123 commented Dec 27, 2024

What changes were proposed in this pull request?

create catalog
image

create schema
image

register/view/drop/list model
image
image

link/view/drop/list versions
image
image
image
image

Why are the changes needed?

(Please clarify why the changes are needed. For instance,

  1. If you propose a new API, clarify the use case for a new API.
  2. If you fix a bug, describe the bug.)

Fix: #5817

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

manually

@jerryshao
Copy link
Contributor

Can you please also add the web UI doc in this PR.

@LauraXia123
Copy link
Collaborator Author

Sure, I updated the commit.

@jerryshao
Copy link
Contributor

The CI is failed, can you please fix the CI, thanks.

@jerryshao jerryshao requested a review from orenccl December 27, 2024 10:59
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this image cannot be displayed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I checked, and the image is displaying normally on my end.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this image correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

docs/assets/webui/list-columns.png is actually correct. Need to click view file to see full image.

docs/webui.md Outdated
3. **Provider**(**_required_**):
1. Type `relational` - `hive`/`iceberg`/`mysql`/`postgresql`/`doris`/`paimon`/`hudi`/`oceanbase`
2. Type `fileset` - `hadoop`
3. Type `messaging` - `kafka`
4. Type `model` have no provider
Copy link
Contributor

Choose a reason for hiding this comment

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

"...has no provider"

Copy link
Member

Choose a reason for hiding this comment

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

If it has no provider, then provider is also not required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Required for type relational /fileset/messaging.

docs/webui.md Outdated Show resolved Hide resolved
docs/webui.md Outdated Show resolved Hide resolved
docs/webui.md Outdated Show resolved Hide resolved
web/web/src/app/metalakes/metalake/MetalakeView.js Outdated Show resolved Hide resolved
}
})
.catch(err => {
console.error('valid error', err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

valid error seems a bit unclear or weird.

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 just for debugging purposes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it.

trigger('aliases')
}}
label={`Aliase ${index + 1}`}
error={!!errors.aliases?.[index]?.name || !!errors.aliases?.message}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why using !!?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The error field accepts a boolean value

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. Thanks.

Copy link
Collaborator

@orenccl orenccl left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for explaining and improving.

Copy link
Collaborator

@orenccl orenccl left a comment

Choose a reason for hiding this comment

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

docs/assets/webui/list-columns.png is actually correct. Need to click view file to see full image.

image

@orenccl orenccl self-requested a review December 30, 2024 06:20
@orenccl
Copy link
Collaborator

orenccl commented Dec 30, 2024

Hi @LauraXia123,
Did you forget to upload the updated docs/webui.md file in the "Improve Docs and Code" commit?

@LauraXia123
Copy link
Collaborator Author

Hi @LauraXia123, Did you forget to upload the updated docs/webui.md file in the "Improve Docs and Code" commit?

sorry, I missed it.

@jerryshao jerryshao merged commit d64c514 into apache:main Dec 30, 2024
23 checks passed
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.

[Subtask]Implement the server rest endpoint for model catalog.
4 participants