Skip to content
This repository was archived by the owner on Apr 23, 2026. It is now read-only.

linux_test - #1337

Merged
mergify[bot] merged 4 commits into
instructlab:mainfrom
RedHat-Israel:linux_test2
Jul 11, 2024
Merged

linux_test#1337
mergify[bot] merged 4 commits into
instructlab:mainfrom
RedHat-Israel:linux_test2

Conversation

@makelinux

@makelinux makelinux commented Jun 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

linux_test is implemented like the chat and the generate with OpenAI client.

Don't use TLS for simplicity, assuming it's a local server only because fine-tuned model is local.

Resolves #659

Testing: #632

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the
    conventional commits.
  • Changelog updated with breaking and/or notable changes for the next minor release.

@mergify mergify Bot added ci-failure PR has at least one CI failure documentation Improvements or additions to documentation and removed ci-failure PR has at least one CI failure labels Jun 12, 2024
@nathan-weinberg

nathan-weinberg commented Jun 12, 2024

Copy link
Copy Markdown
Contributor

@makelinux is this a duplicate of #1314?

Adding hold until this is answer

@nathan-weinberg nathan-weinberg added the hold In-progress PR. Tag should be removed before merge. label Jun 12, 2024
@makelinux

makelinux commented Jun 12, 2024

Copy link
Copy Markdown
Contributor Author

@nathan-weinberg , no, it is another better method

@nathan-weinberg

nathan-weinberg commented Jun 12, 2024

Copy link
Copy Markdown
Contributor

@makelinux in the future, please limit changes to a single PR - if you are experimenting with implementations, you can open a draft PR and open it for review once it is ready

@nathan-weinberg nathan-weinberg removed the hold In-progress PR. Tag should be removed before merge. label Jun 12, 2024
@mergify mergify Bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jun 12, 2024
@nathan-weinberg
nathan-weinberg requested a review from a team June 12, 2024 18:27
@mergify mergify Bot added the needs-rebase This Pull Request needs to be rebased label Jun 13, 2024
@mergify

mergify Bot commented Jun 13, 2024

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. @makelinux please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot removed the needs-rebase This Pull Request needs to be rebased label Jun 13, 2024
@russellb
russellb self-requested a review June 13, 2024 16:26
russellb
russellb reviewed Jun 13, 2024
View reviewed changes
Comment thread src/instructlab/model/linux_test.py Outdated
@mergify mergify Bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jun 14, 2024
leseb
leseb suggested changes Jun 14, 2024
View reviewed changes
Comment thread src/instructlab/model/test.py
Comment thread src/instructlab/model/test.py Outdated
Comment thread src/instructlab/model/test.py Outdated
Comment thread src/instructlab/model/test.py Outdated
Comment thread src/instructlab/model/test.py Outdated
Comment thread src/instructlab/model/test.py Outdated
Comment thread src/instructlab/model/test.py Outdated
Comment thread src/instructlab/model/test.py
@mergify mergify Bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jun 14, 2024
@makelinux
makelinux requested a review from leseb June 14, 2024 15:11
@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Jul 10, 2024
@leseb
leseb requested review from tiran and removed request for chudegao July 10, 2024 13:57
@mergify mergify Bot added the needs-rebase This Pull Request needs to be rebased label Jul 10, 2024
@mergify

mergify Bot commented Jul 10, 2024

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. @makelinux please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added needs-rebase This Pull Request needs to be rebased and removed needs-rebase This Pull Request needs to be rebased labels Jul 10, 2024
@mergify

mergify Bot commented Jul 10, 2024

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. @makelinux please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added ci-failure PR has at least one CI failure needs-rebase This Pull Request needs to be rebased and removed needs-rebase This Pull Request needs to be rebased ci-failure PR has at least one CI failure labels Jul 11, 2024
@mergify

mergify Bot commented Jul 11, 2024

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. @makelinux please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

makelinux added 4 commits July 11, 2024 11:44
@makelinux
http_client: allow undefined TLS params
9f7d391 
Prepare for linux_test().

Connection to local server for testing is possible without TLS.

Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
@makelinux
replace macos_requirement with is_macos_with_m_chip in test()
2bdd54c 
No code changes, just change in indentation.

Preparation for linux_test().

Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
@makelinux
linux_test
c9d7349 
linux_test is implemented like the chat and the generate
with OpenAI client.

Only `ensure_server` and starting the server
defines `model_path`, client.chat.completions.create ignores it.

Don't use TLS for simplicity, assuming it's a local server only because
fine-tuned model is local.

Resolves #659

Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
@makelinux
functional-tests.sh: add test of ilab model test
005e0ad 
test_model_train() emulates very heavy `ilab model train'
just to make work for `test_model_test()`.

Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
@mergify mergify Bot removed the needs-rebase This Pull Request needs to be rebased label Jul 11, 2024
RobotSail
RobotSail approved these changes Jul 11, 2024
View reviewed changes

@RobotSail RobotSail left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for your contribution, LGTM!

Comment thread src/instructlab/model/backends/backends.py
from .vllm import Server as vllm_server

model_path = pathlib.Path(cfg.model_path)
model_path = pathlib.Path(model_path or cfg.model_path)

@RobotSail RobotSail Jul 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not going to block on this but just as an FYI you should prefer to use cfg.model_path if not model_path else model_path in python when doing these default evaluations.

@mergify mergify Bot removed the one-approval PR has one approval from a maintainer label Jul 11, 2024
@leseb
leseb removed request for russellb and tiran July 11, 2024 12:44
@mergify
mergify Bot merged commit fcf72f6 into instructlab:main Jul 11, 2024
@ktam3 ktam3 added this to the 0.18.0 milestone Jul 15, 2024
makelinux added a commit to makelinux/instructlab that referenced this pull request Aug 3, 2024
@makelinux
doc: remove "testing doesn't work on Linux"
32a2191 
Already supported, see instructlab#1337

Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
@makelinux makelinux mentioned this pull request Aug 3, 2024
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers
@russellb russellb russellb left review comments
@chudegao chudegao chudegao left review comments
@leseb leseb leseb approved these changes
@RobotSail RobotSail RobotSail approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

documentation Improvements or additions to documentation testing Relates to testing

Projects

None yet

Milestone

0.18.0a1

Development

Successfully merging this pull request may close these issues.

lab test should be implemented for Linux

7 participants

@makelinux @nathan-weinberg @leseb @russellb @chudegao @RobotSail @ktam3