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

Don't mount model dir for no code tests #676

Merged
merged 1 commit into from
Apr 28, 2023
Merged

Don't mount model dir for no code tests #676

merged 1 commit into from
Apr 28, 2023

Conversation

siddvenk
Copy link
Contributor

Description

Test run https://github.com/deepjavalibrary/djl-serving/actions/runs/4834079393/jobs/8614905302.

Although it failed, the issue was with the model name when it was registered (default is model, we expect test). I updated the workflow to match the expected model name on the client side.

Functionally, the test run showed these changes worked.

@siddvenk siddvenk requested review from zachgk, frankfliu and a team as code owners April 28, 2023 21:53
./launch_container.sh deepjavalibrary/djl-serving:$DJLSERVING_DOCKER_TAG $PWD/models deepspeed \
serve -m test=file:/opt/ml/model/test/
./launch_container.sh deepjavalibrary/djl-serving:$DJLSERVING_DOCKER_TAG no_code deepspeed \
serve -m test=file:/opt/ml/model/
Copy link
Contributor

Choose a reason for hiding this comment

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

remove -m, we also don't need this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we remove this, is there a way to specify the model name as test? I initially removed this and then ran into issues with client.py expecting the model name to be test, but by default the model will be called model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can definitely update client.py, but I thought this was easier

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, let's leave this now

Copy link
Contributor

Choose a reason for hiding this comment

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

We need specify the model name here. we don't have a way to call management API without model name (this is not supported by SageMaker).

@siddvenk siddvenk merged commit 8a4d4ec into master Apr 28, 2023
@siddvenk siddvenk deleted the no-code-tests branch April 28, 2023 22:50
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.

3 participants