Skip to content

Conversation

jeffmaury
Copy link
Collaborator

Fixes #2789

What does this PR do?

Update model info after download/upload is complete

Screenshot / video of UI

N/A

What issues does this PR fix or reference?

#2789

How to test this PR?

Start a recipe with model download included

@jeffmaury jeffmaury requested review from benoitf and a team as code owners April 1, 2025 10:12
Copy link
Contributor

@gastoner gastoner left a comment

Choose a reason for hiding this comment

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

Codewise LGTM, but can't we extract this code to the extra function since it is used in multiple places? So we would avoid "don't repeat yourself" principle?

// refresh model lists on event completion
    await this.getLocalModelsFromDisk();
    this.sendModelsInfo().catch((err: unknown) => {
      console.error('Something went wrong while sending models info.', err);
    });

@jeffmaury
Copy link
Collaborator Author

Codewise LGTM, but can't we extract this code to the extra function since it is used in multiple places? So we would avoid "don't repeat yourself" principle?

// refresh model lists on event completion
    await this.getLocalModelsFromDisk();
    this.sendModelsInfo().catch((err: unknown) => {
      console.error('Something went wrong while sending models info.', err);
    });

Fixed

@benoitf benoitf requested a review from Copilot April 1, 2025 11:04
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue (#2789) where model info was updated asynchronously causing problems during recipe start. The changes refactor the model info update logic by extracting the refresh code into a new updateModelInfos method and ensuring it's called after both download and upload operations.

Comments suppressed due to low confidence (2)

packages/backend/src/managers/modelsManager.ts:400

  • [nitpick] Consider renaming updateModelInfos to refreshModelInfos for clarity, to better convey that the method refreshes the model information.
private async updateModelInfos(): Promise<void> {

packages/backend/src/managers/modelsManager.ts:394

  • [nitpick] Consider renaming the variable 'path' to 'uploadPath' to more clearly indicate its purpose as the target path returned from the uploader.
const path = uploader.perform(model.id);

@jeffmaury jeffmaury merged commit e9c1a5b into containers:main Apr 1, 2025
7 checks passed
@jeffmaury jeffmaury deleted the GH-2789 branch April 1, 2025 12:30
@ScrewTSW
Copy link
Member

ScrewTSW commented Apr 1, 2025

GPU enabled deployment fails the same way as for this PR #2708 (comment)

CPU inference AOK

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.

Can't start a recipe when model has to be downloaded
5 participants