-
Notifications
You must be signed in to change notification settings - Fork 78
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
Chains Smoke Tests #1298
Chains Smoke Tests #1298
Conversation
b76ea4f
to
af31f7f
Compare
00fc951
to
f2d53e2
Compare
af31f7f
to
99edfd2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM -- I think after merging this it'll be much easier to iterate on this. Most of my comments relate to how we're invoking chains and whether it would be possible to use the Python SDK
return stub.StubBase.from_url(url, context, options) | ||
|
||
|
||
def write_trussrc(dir_path: pathlib.Path) -> pathlib.Path: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[non-blocking] maybe we should extend truss.login
to support these other params.... https://docs.baseten.co/truss-reference/python-sdk#truss-login
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm - is there a customer use case or is this only relevant for your internal dev?
tmpdir, truss_rc_path, remote = prepare | ||
|
||
chain_src = CHAINS_ROOT / "tests" / "itest_chain" / "itest_chain.py" | ||
command = f"truss chains push {chain_src} --publish --name=itest_publish --no-wait" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to other comments other here, feels like a good opportunity to improve the truss python sdk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean - does #1298 (comment) clarify it though?
env: | ||
TRUSS_ENV_PATH: ${{ github.workspace }}/truss_env | ||
run: | | ||
BASETEN_API_KEY_STAGING="${{ secrets.BASETEN_API_KEY_STAGING }}" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what account are we using for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
marius.killinger+staging@baseten.co
Can we somehow create name+PW accounts on staging? Then I could create a dedicated account and add it to onePW. But in a way it doesn't really matter, since everyone can just hijack anything on staging.
return stdout, stderr | ||
|
||
|
||
def wait_ready( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use this wait_for_active() thing: https://docs.baseten.co/truss-reference/python-sdk#wait-for-active
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't. There is by intention an "air gap" between the system under test (the truss CLI installed in a separate env) and the poetry env that orchestrates the tests. One reason for that is that we want to test a pip, non-dev user install, i.e. making sure that CLI code branches don't accidentally depend on dev-only dependencies (we a few times bad truss releases where something passed our CI tests, but in the user install a dependency was missing).
For the same reason, I don't want to use the python SDK, but instead make sure the CLI works as most users would use it - there was so far 0 testing for the CLI, the SDK is mostly covered by existing integration tests.
adcde4c
to
d788900
Compare
🚀 What
E2E deployment and invocation of a published chain. The primary test focus is the truss package with CLI (not the test chain code and not baseten backend).
💻 How
🔬 Testing
Locally and testing the CI workflow here: https://github.com/basetenlabs/truss/actions/runs/12641883558
Note that for testing I had to
a) Add another trigger, so that the workflow can be run, while it's not merged to main. This trigger is removed from the final PR version. Until it's merged, the WF cannot be run again in this state.
b) Create a RC from #1296 and hardcode it in the worklow (also removed in final PR) because the tests need overridable truss rc paths.