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

chore: changed destination of ai lab port settings #2052

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

Conversation

gastoner
Copy link
Contributor

@gastoner gastoner commented Nov 5, 2024

What does this PR do?

Changes destination of ai lab port settings

Screenshot / video of UI

image

What issues does this PR fix or reference?

Closes #1978

How to test this PR?

Go to AI Lab, there should be a new navigation item with the settings

@gastoner
Copy link
Contributor Author

gastoner commented Nov 5, 2024

TODO: add unit test

@benoitf
Copy link
Collaborator

benoitf commented Nov 5, 2024

question: it's not clear to me in the issue if it means that the configuration should be removed from the classic configuration page of any extension.
IMHO it feels an anti-pattern if it's the case

@benoitf
Copy link
Collaborator

benoitf commented Nov 5, 2024

side comment: also it's possible to provide a markdown description in the properties. So it looks like we're trying to reinvent the wheel of how each extension will be configured

@gastoner
Copy link
Contributor Author

gastoner commented Nov 5, 2024

cc @slemeur

@@ -44,13 +44,6 @@
"default": false,
"description": "Experimental GPU support for inference servers"
},
"ai-lab.apiPort": {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be removed

@slemeur
Copy link
Contributor

slemeur commented Nov 6, 2024

@benoitf: The intent was to expose the REST Server capabilities from Podman AI Lab, by providing its own page. Right now, it would only explain what it does and provides the ability to configure the server port.

In the future the user could in the future:

  • start/stop the local server
  • access to end points API documentation
  • access the logs

Does that make sense to you?

Considering your feedback, I agree that the "Configurable Options" is going to be confusing.

Maybe, we should call the section: "Local Server" and the subpage "Server Information", wdyt?

@benoitf
Copy link
Collaborator

benoitf commented Nov 6, 2024

@slemeur my point was that any configuration should stay at a minimum in the "generic configuration" settings (what is being exposed by an extension)

then the extension can still decide to expose it through a different UI, but being "on top of the configuration settings"

so, more keep it at both place (if I go to all settings and filter for AI Lab extension, I should be able to find something there)

@slemeur
Copy link
Contributor

slemeur commented Nov 6, 2024

Ok that makes sense indeed.

@slemeur
Copy link
Contributor

slemeur commented Nov 6, 2024

@gastoner: I've updated the initial issue.

Here are the changes for your PR:

  • Section title "Local Server"
  • Dedicated page "Server Information"
  • We will be keeping the possibility to change the Server Port in the settings of Podman AI Lab extension as well

@gastoner gastoner force-pushed the move_ai_lab_listening_port_to_its_own_configuration_page branch from fc113fb to 2dadc21 Compare November 11, 2024 12:22
@gastoner gastoner force-pushed the move_ai_lab_listening_port_to_its_own_configuration_page branch from 8a5df4a to 085e635 Compare November 11, 2024 13:18
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.

Move the AI Lab listening port in its own configuration page
4 participants