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: test InstructLab in a container #2292

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

Conversation

jeffmaury
Copy link
Contributor

@jeffmaury jeffmaury commented Dec 23, 2024

What does this PR do?

Fixes #2226

Allow to start InstructLab container and open a terminal

Screenshot / video of UI

ilab

What issues does this PR fix or reference?

#2226

How to test this PR?

  1. Start Podman AI Lab
  2. Select the Test InstructLab menu
  3. Click the Start InstructLab container button
  4. Click the Open InstructLab container button

@jeffmaury jeffmaury requested a review from slemeur December 23, 2024 16:24
@jeffmaury jeffmaury force-pushed the GH-2226 branch 2 times, most recently from aa857a4 to 0df95a8 Compare December 23, 2024 16:52
@slemeur
Copy link
Contributor

slemeur commented Dec 24, 2024

👍 Thanks @jeffmaury for this. It's looking great and the ability to open the terminal directly will be helpful.

Couple of suggestions:

  • Can we separate the instructions from the box that allows to start the container?
  • Once the container is started, could we have the button saying "Open Instruct Lab terminal" ?

@jeffmaury jeffmaury marked this pull request as ready for review January 2, 2025 10:48
@jeffmaury jeffmaury requested review from benoitf and a team as code owners January 2, 2025 10:48
@axel7083 axel7083 self-requested a review January 2, 2025 14:24
{/if}
<Button title="Read documentation" type="link" on:click={submit}>Read documentation</Button>
</div>
</header>
Copy link
Contributor

Choose a reason for hiding this comment

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

image

A bit weird to have the buttons on the top ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See @slemeur

Copy link
Contributor

Choose a reason for hiding this comment

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

When the action is not started, the buttons are at the top.
Once the user clicked on the "Start" button, the triggered actions display bellow the action, which seems the natural reading order and avoid the page's primary actions to be moving in the screen.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not really consistent with the other forms where the buttons are on the bottom right

Copy link
Contributor

Choose a reason for hiding this comment

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

But that's not a form. There is only one action the user can do on this screen.

Copy link
Contributor

@axel7083 axel7083 left a comment

Choose a reason for hiding this comment

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

We might want to create a task with a goto action, as when leaving the page we cannot go back, the pulling can take a lot of time.. so might be nice to be able to leave the page and go back to it.)

I am open to have it as a separate task, (let's add it to the epic #2224 ?

Signed-off-by: Jeff MAURY <[email protected]>
Signed-off-by: Jeff MAURY <[email protected]>
@slemeur
Copy link
Contributor

slemeur commented Jan 3, 2025

@jeffmaury : to confirm, is the telemetry triggered when the user clicks on the "start" button?

Signed-off-by: Jeff MAURY <[email protected]>
@jeffmaury
Copy link
Contributor Author

@jeffmaury : to confirm, is the telemetry triggered when the user clicks on the "start" button?

Fixed by 9e7822d

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.

On Windows:
Invisible text in light theme
image
+1 Axel, I would expect also the buttons on bottom right

The cancel button does not do anything, the page have a different template?
https://github.com/user-attachments/assets/7a40398b-fe6a-485d-921f-a697bff9f11f

@axel7083
Copy link
Contributor

axel7083 commented Jan 8, 2025

I was able to try it on Linux and I am getting permission issues

bash-4.4$ ilab config init
Traceback (most recent call last):
  File "/usr/local/bin/ilab", line 8, in <module>
    sys.exit(ilab())
             ^^^^^^
  File "/usr/local/lib/python3.11/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/instructlab/clickext.py", line 319, in wrapper
    return f(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/click/decorators.py", line 33, in new_func
    return f(get_current_context(), *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/instructlab/config/init.py", line 100, in init
    fresh_install = ensure_storage_directories_exist()
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/instructlab/configuration.py", line 1122, in ensure_storage_directories_exist
    os.makedirs(dirpath, exist_ok=True)
  File "<frozen os>", line 225, in makedirs
PermissionError: [Errno 13] Permission denied: '/instructlab/.local/share/instructlab/chatlogs'
bash-4.4$

This is probably due to a missing label/configuration on the mounted folder(s).

Signed-off-by: Jeff MAURY <[email protected]>
@jeffmaury jeffmaury requested a review from gastoner January 14, 2025 07:13
Co-authored-by: axel7083 <[email protected]>
Signed-off-by: Jeff MAURY <[email protected]>
Copy link
Contributor

@axel7083 axel7083 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

When I try to delete the container and the image after the start+open.
Then go back to try instructlab, the start button is starting container again

Screen.Recording.2025-01-14.111620.mp4

Start InstructLab container
</Button>
{/if}
<Button title="Read documentation" type="link" on:click={openDocumentation}>Read documentation</Button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Add some space between buttons?
image

@gastoner
Copy link
Contributor

Going to Tune with instructlab will stop showing the in progress button:

Screen.Recording.2025-01-14.111243.mp4

Signed-off-by: axel7083 <[email protected]>
Copy link
Contributor

@axel7083 axel7083 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Provide ability to start InstructLab Container
4 participants