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

api v1alpha1 #17

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

api v1alpha1 #17

wants to merge 22 commits into from

Conversation

guimou
Copy link

@guimou guimou commented Jan 9, 2025

Rework for API version v1alpha1 as discussed.

@PeterStaar-IBM
Copy link

@dolfim-ibm Let's prioritize this PR asap!

guimou and others added 5 commits January 24, 2025 14:13
Signed-off-by: Guillaume Moutier <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
Github Action runners are running out of the space while
building both the images in parallel.

This change will build the image sequentially and also
clean up the cpu images before start building gpu image.

Signed-off-by: Anil Vishnoi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
@dolfim-ibm
Copy link
Contributor

@guimou I started with a few changes

  • used the Annotated syntax for the parameters
  • use default values and not Optional (which in Python means "can be None" and has almost no relation with the concept of optional)
  • refactor the request arguments logic. where possible use the native enum.
  • make the gradio part optional (but highly advised)
  • rebase with main such that tests build the image

Next, we will look

  • the background processing
  • input/output schemas

@guimou
Copy link
Author

guimou commented Jan 24, 2025 via email

@dolfim-ibm
Copy link
Contributor

@guimou I reworked a few parts, I think that now the PR is ready for merge and release.

@vishnoianil do you want to also have a quick look?

dolfim-ibm
dolfim-ibm previously approved these changes Jan 27, 2025
Copy link
Contributor

@dolfim-ibm dolfim-ibm left a comment

Choose a reason for hiding this comment

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

great


EXPOSE 5000
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 any specific need to run this service on port 8080? I am wondering If we can revert it 5000 or some other non-80** port? @dolfim-ibm

Copy link
Author

Choose a reason for hiding this comment

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

It was to run on the same "standard" port as other serving runtimes in OpenShift AI. But it's an env var, so can be anything in the end.

@vishnoianil
Copy link
Collaborator

Awesome work @guimou

@dolfim-ibm minor comment, PR looks good to me. I am wondering can we enable the arm64 arc for "cpu only" image ? It's enabled for GPU image, but not for CPU only image. I remember we were discussing about some dependencies not available for arm64, and that was causing issue in building arm64 image, is that still true?

Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
@dolfim-ibm
Copy link
Contributor

I realized we missed addressing the input http with custom headers. I'm making a pushing a version similar to what we have on origin/main.

Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
@PeterStaar-IBM PeterStaar-IBM self-requested a review January 28, 2025 12:20
@dolfim-ibm
Copy link
Contributor

@vishnoianil The PORT has been changed to 5001. Additionally I also reworked the input payload to match the format already in main which allows custom headers and base64 input.

Copy link

@PeterStaar-IBM PeterStaar-IBM left a comment

Choose a reason for hiding this comment

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

let's go!

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.

4 participants