-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add paddle-server ROCK rocraft #20
Conversation
@misohu shouldn't we add also the CI automation in the repo for the ROCKs to get build/published on merge? |
I am +1 to adding the CI @kimwnasptd, but adding the central files (anything in .github) should be done in a separate PR. Adding the files that are needed in this particular rock (like a tox.ini) should be done in this PR The settings on this repo are also not correct (I see there's no branch protection, for example). I'll split it with you michal - you add the CI, I'll change the settings :D |
Looking at the parent issue #18, it does explicitly say publish manually. But I'm with @kimwnasptd in that I think we should change it to be automatic |
Added the tests and tox file for paddle, resolved @ca-scribner comments (replied to questions) opened issue #21 for CI. |
@ca-scribner @kimwnasptd here is PR for CI #22 |
95c3588
to
a7aa1dc
Compare
I'll give this another review, but I'm still messing around with things in my related issue #19. I think one or two things here might be redundant (maybe rockcraft changed since last we did this?) |
This change ensures that `python` is executable, the kserve/paddlserver packages are installed the same as other packages, and adds commends to the rockcraft file.
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.
Everything looks good, just a super small comment. Thanks!
Steps to test
There should be an output with deprecation warnings like this
The output from workload service is identical to upstream container