-
Notifications
You must be signed in to change notification settings - Fork 41
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
fix: upload model on podman machine on WSL to speed loading #204
Conversation
274b4f5
to
f944d7c
Compare
f944d7c
to
5b4daf4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only concern I have is that it uses the default Podman machine which may be stopped and deployments are performed against the first Podman machine that is running
@jeffmaury updated |
d088ce3
to
c957f6d
Compare
c957f6d
to
3529ee7
Compare
* SPDX-License-Identifier: Apache-2.0 | ||
***********************************************************************/ | ||
|
||
export interface ProgressiveEvent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why you renamed from ProgressEvent
to ProgressiveEvent
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are two different things.
Ai studio had the DownloadEvent. This PR is adding an upload action which would require a similar event and so i renamed the DownloadEvent
to ProgressiveEvent
. It's like downloadEvent and uploadEvent = progressiveEvent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like Progress
and Progressive
are too close and could lead to some confusion, we might want to find something different to improve readability.
BaseEvent
instead of DownloadEvent
and keep ProgressEvent
to talk about the real progress.
or
TransferEvent
if we want to keep some action/wording of what is happening
I have a preference for BaseEvent, as we could create its own
IBaseEvent
class, and keep it separate from the logic of the Downloader/Uploader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
progressing = happening or developing gradually or in stages. Look to me that it expresses well what it does and it can be reused.
By using TrasferEvent we are binding it only to transferring actions which may not be true in future, no?
Maybe ExecutingProgressiveEvent
instead of ProgressProgressiveEvent
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is the Download
event
export interface DownloadEvent {
id: string;
status: 'error' | 'completed' | 'progress' | 'cancelled';
message?: string;
}
This seems very basic and generic, I still think something short and easy to read like BaseEvent
would be more suitable than ExecutingProgressiveEvent
... but this is just my opinion, if other people are fine with your choice I open to accept it !
04aaf81
to
b30b71d
Compare
@jeffmaury rebased |
No, have you done something in particular? I'll try now |
🤦 no, sorry. Pushed now |
lucas-pr.mp4Maybe my repo is too old ? (will retry after I ate) |
You can try, the error seems related to the podman machine though 🤔 I'll add a check to prevent this kind of error |
|
Only that machine? I wonder why you don't have it set to default. Podman desktop should ask you to set the active machine as the default one. |
Apparently it is not, but this case should be handle I guess |
Signed-off-by: lstocchi <[email protected]>
Signed-off-by: lstocchi <[email protected]>
Signed-off-by: lstocchi <[email protected]>
Signed-off-by: lstocchi <[email protected]>
Signed-off-by: lstocchi <[email protected]>
Signed-off-by: lstocchi <[email protected]>
Signed-off-by: lstocchi <[email protected]>
Signed-off-by: lstocchi <[email protected]>
Signed-off-by: lstocchi <[email protected]>
4a41457
to
b582f66
Compare
I was able to replicate your case by using rootful machine. |
throw new Error('No podman machine is running'); | ||
} | ||
// check if model already loaded on the podman machine | ||
let existsRemote = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In another PR, that would be better to have this as a callable method, so we could allow from the model list to upload it to the podman machine manually by the user. We would also probably want to be able to delete them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I agree 👍
Once uploaded, the model is soo fast to start !! Love it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Great improvment
Signed-off-by: lstocchi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested and works fine ! Great job 👏 !!
So, it should be ready.
When you are on WSL the model gets uploaded to the podman machine. This makes us loses like 20sec during the start of application but it makes us gain minutes when the model is actually loaded by the model service.
So to test it,
podman machine ssh ls
If the user system is no Windows, it works as before. Qemu will be added on another PR