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

feat: display notification when upload model by URL #126

Conversation

ruanyl
Copy link
Member

@ruanyl ruanyl commented Feb 24, 2023

  • refactor hook based model task polling function with rxjs

Signed-off-by: Yulong Ruan ruanyl@amazon.com

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@ruanyl ruanyl requested a review from wanglam February 24, 2023 02:50
public/components/register_model/model_task_manager.ts Outdated Show resolved Hide resolved
// 1. model id doesn't exist
// 2. the current task is running fine without error
if (!res.model_id && !res.error) {
this.tasks.next(this.tasks.getValue().set(options.taskId, observable));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we append this tasks outside? From the code I think this setter will be called after first get task detail API response, it may cause race condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated so that it only set the task once.

My intention was only adding the running task to this.task, if we know a task is completed or has error in the first call, then we don't add it to this.tasks. So that we can know the current running task count by subscribe to this.tasks.

Btw, what's the race condition you mentioned?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm misunderstanding here. What I originally wanted to ask is that there is a time gap from the user calls the query method to the tasks add the task to the map. If you access the tasks map within this time gap, user won't get the corresponding task. Do we need to concern about this problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

this.tasks can be converted to observable, see this.getTasks$, user can subscribe to the observable to get the changes

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure. Is that means the changes notified after first get task detail API called? I think it's OK if these is intention.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was thinking of only pushing it to tasks when we know the task is in progress from the first API response. So this.tasks represents the tasks which are in progress.

public/components/register_model/model_task_manager.ts Outdated Show resolved Hide resolved
public/components/register_model/model_task_manager.ts Outdated Show resolved Hide resolved
public/components/register_model/model_task_manager.ts Outdated Show resolved Hide resolved
refactor model upload hook by rxjs

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>
refactor hook based model task polling function with rxjs

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>
Signed-off-by: Yulong Ruan <ruanyl@amazon.com>
Signed-off-by: Yulong Ruan <ruanyl@amazon.com>
@ruanyl ruanyl force-pushed the model-upload-by-url-notification branch from 4bdc07e to a11907e Compare February 27, 2023 06:26
@ruanyl ruanyl merged commit 9eefbc2 into opensearch-project:feature/model-registry Feb 27, 2023
@ruanyl ruanyl deleted the model-upload-by-url-notification branch February 27, 2023 06:36
wanglam pushed a commit to wanglam/ml-commons-dashboards that referenced this pull request Jan 3, 2025
…ct#126)

+ polling model upload task status and show notifications if task failed/successful
+ removed useModelUpload hook and refactor it with rxjs

---------

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
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