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

Package update + more docs on dev setup #500

Merged
merged 4 commits into from
Apr 24, 2024
Merged

Conversation

dmchoiboi
Copy link
Collaborator

@dmchoiboi dmchoiboi commented Apr 23, 2024

Pull Request Summary

What is this PR changing? Why is this change being made? Any caveats you'd like to highlight? Link any relevant documents, links, or screenshots here if applicable.

  • Upgrade poetry to 1.8.2 to fix package version conflicts with pip-tools in requirements-dev.txt
  • Add docs on setting to WORKSPACE env var to be able to run tests locally
  • Fix faulty type: ignore and handle type narrowing explicitly
  • Add test case for high priority + zipartifact codepath for test coverage

Test Plan and Usage Guide

How did you validate that your PR works correctly? How do you run or demo the code? Provide enough detail so a reviewer can reasonably reproduce the testing procedure. Paste example command line invocations if applicable.
Tested pip install + mypy + pytest on a fresh venv using python 3.8.10

# new venv
python -m venv venvs/llm-engine
source ./venvs/llm-engine/bin/activate

# install packages
pip install -r ../requirements-dev.txt
pip install -r requirements.txt && \
    pip install -r requirements-test.txt && \
    pip install -r requirements_override.txt && \
    pip install -e .

# typecheck
mypy . --install-types

# unit tests
pytest

Copy link
Member

@seanshi-scale seanshi-scale left a comment

Choose a reason for hiding this comment

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

lgtm if integration tests pass, looks like some code coverage tests failed? guess we'll need to fix that too

current_model_bundle=model_bundle_3,
owner=test_api_key,
),
high_priority=True,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add this fixture to test the high_priority code path

@dmchoiboi dmchoiboi merged commit 9673b3f into main Apr 24, 2024
5 checks passed
@dmchoiboi dmchoiboi deleted the dmchoi/package-bumps branch April 24, 2024 05:32
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.

None yet

3 participants