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

playground tests #96

Merged
merged 3 commits into from
Mar 28, 2024
Merged

playground tests #96

merged 3 commits into from
Mar 28, 2024

Conversation

lmilbaum
Copy link
Collaborator

No description provided.

@lmilbaum lmilbaum force-pushed the playground-tests branch 2 times, most recently from 3efcf80 to e68f7e0 Compare March 24, 2024 21:20
@lmilbaum
Copy link
Collaborator Author

lmilbaum commented Mar 24, 2024

The following test throws an error which I'm still debugging:

================================================================================ ERRORS =================================================================================
____________________________________________________________ ERROR collecting playground/tests/test_alive.py ____________________________________________________________
/opt/app-root/lib64/python3.11/site-packages/pluggy/_hooks.py:501: in __call__
    return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
/opt/app-root/lib64/python3.11/site-packages/pluggy/_manager.py:119: in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
/opt/app-root/lib64/python3.11/site-packages/_pytest/python.py:276: in pytest_pycollect_makeitem
    return list(collector._genfunctions(name, obj))
/opt/app-root/lib64/python3.11/site-packages/_pytest/python.py:489: in _genfunctions
    self.ihook.pytest_generate_tests.call_extra(methods, dict(metafunc=metafunc))
/opt/app-root/lib64/python3.11/site-packages/pluggy/_hooks.py:562: in call_extra
    return self._hookexec(self.name, hookimpls, kwargs, firstresult)
/opt/app-root/lib64/python3.11/site-packages/pluggy/_manager.py:119: in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
playground/tests/conftest.py:7: in pytest_generate_tests
    pytest_container.auto_container_parametrize(metafunc)
/opt/app-root/lib64/python3.11/site-packages/pytest_container/helpers.py:36: in auto_container_parametrize
    raise ValueError(
E   ValueError: The test function test_alive is using the auto_container fixture but the parent module is not setting the 'CONTAINER_IMAGES' variable
======================================================================== short test summary info ========================================================================
ERROR playground/tests/test_alive.py - ValueError: The test function test_alive is using the auto_container fixture but the parent module is not setting the 'CONTAINER_IMAGES' variable

@lmilbaum lmilbaum self-assigned this Mar 24, 2024
@lmilbaum lmilbaum force-pushed the playground-tests branch 19 times, most recently from 3f69279 to bad157b Compare March 25, 2024 17:22
@lmilbaum lmilbaum force-pushed the playground-tests branch 3 times, most recently from ebc1242 to 7263321 Compare March 25, 2024 18:47
@lmilbaum lmilbaum requested a review from dcermak March 27, 2024 17:45
Copy link
Contributor

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

I think this makes sense. Overall comment is it could use a brief high level architecture description (possibly in the Makefile?) or a separate readme.md.

playground/Makefile Outdated Show resolved Hide resolved
playground/tests/conftest.py Outdated Show resolved Hide resolved
playground/Makefile Outdated Show resolved Hide resolved
playground/tests/conftest.py Outdated Show resolved Hide resolved
@lmilbaum lmilbaum force-pushed the playground-tests branch 5 times, most recently from 645d10d to 0be53af Compare March 27, 2024 19:10
@lmilbaum lmilbaum requested a review from cgwalters March 27, 2024 19:14
@rhatdan
Copy link
Member

rhatdan commented Mar 27, 2024

@MichaelClifford PTAL

@lmilbaum lmilbaum force-pushed the playground-tests branch 3 times, most recently from f218203 to c590fe6 Compare March 28, 2024 11:50
@lmilbaum lmilbaum requested a review from dcermak March 28, 2024 11:52
lmilbaum and others added 2 commits March 28, 2024 10:37
Signed-off-by: Liora Milbaum <[email protected]>
Signed-off-by: sallyom <[email protected]>
playground/Makefile Outdated Show resolved Hide resolved
playground/Makefile Outdated Show resolved Hide resolved
pip install -r playground/tests/requirements.txt

.PHONY: run
run: install models/llama-2-7b-chat.Q5_K_S.gguf
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
run: install models/llama-2-7b-chat.Q5_K_S.gguf
run: install download

playground/Makefile Outdated Show resolved Hide resolved
pip install -r playground/tests/requirements.txt

.PHONY: run
run: install models/llama-2-7b-chat.Q5_K_S.gguf
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 a way to check if the model exists before downloading it? We don't want people re-downloading the model each time the use make run

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. It is possible. I would prefer implementing it in a followup PR if that is not a critical change.

podman build -f playground/Containerfile -t ghcr.io/ai-lab-recipes/playground --format docker playground

models/llama-2-7b-chat.Q5_K_S.gguf:
curl -s -S -L -f https://huggingface.co/TheBloke/Llama-2-7B-Chat-GGUF/resolve/main/llama-2-7b-chat.Q5_K_S.gguf -z $@ -o [email protected] && mv -f [email protected] $@ 2>/dev/null || rm -f [email protected] $@
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use this instead? https://huggingface.co/TheBloke/Mistral-7B-v0.1-GGUF/resolve/main/mistral-7b-v0.1.Q2_K.gguf
Its smaller model (3gb) and has an apache license.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is doable. But, I would prefer implementing it on a followup PR.

@lmilbaum lmilbaum force-pushed the playground-tests branch 7 times, most recently from 45fa1ea to 297fab2 Compare March 28, 2024 16:39
Copy link
Collaborator

@sallyom sallyom left a comment

Choose a reason for hiding this comment

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

LGTM!! Thank you @lmilbaum this is really great and so much work to keep updating with the repo restructuring!!

@sallyom sallyom merged commit 15929b9 into containers:main Mar 28, 2024
3 checks passed
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.

6 participants