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

add sklearnserver rock #23

Merged
merged 6 commits into from
Feb 21, 2024
Merged

add sklearnserver rock #23

merged 6 commits into from
Feb 21, 2024

Conversation

ca-scribner
Copy link
Contributor

@ca-scribner ca-scribner commented Feb 15, 2024

adds a rock for the kserve v0.11 sklearnserver based on this dockerfile and the existing Paddle and pmmlserver rocks

This PR refactors a little compared to the previous rocks like pmmlserver. The main goals were:

  • improve readability (the pmmlserver uses a lot of magic but doesn't explain why/how it works - I've tried to add comments here)
  • make the image API interchangable with upstream as much as possible (no need for pythonpath or calling python3)

Specifically for the pythonpath, the pmmlserver rock needs to add /pmmlserver:/kserve to its service PYTHONPATH because poetry by default installs a root project as editable, which means the code is not stored in dist-packages like the other installed packages. So when we copy our installed python packages from build env to the final rock in pmmlserver, we omit the pmmlserver and kserve packages (and then we "fix" that by copying the code separately, and adding the new code dirs to PYTHONPATH.

I'm worried this PYTHONPATH workaround might have unintended consequences, so instead in this rock I install kserve and sklearnserver packages as local dependencies of a dummy poetry project rather than as root projects. This tricks poetry into installing them normally (not editable), which means they are put in dist-packages like everything else.

Testing instructions

cd sklearnserver
rockcraft pack
sudo skopeo --insecure-policy copy oci-archive:sklearnserver_0.11.0_amd64.rock docker-daemon:charmedkubeflow/sklearnserver:0.11.0
docker run charmedkubeflow/sklearnserver:0.11.0

The rock should launch, but then fail with:

2024-02-15T20:42:59.351Z [sklearnserver] main.py: error: the following arguments are required: --model_dir

adds a rock for the kserve v0.11 sklearnserver.  This rock is modelled after the Paddle and pmmlserver rocks
@ca-scribner
Copy link
Contributor Author

PR is missing the tox.ini/tests folder. will add these next before merging this. But the rock itself works afaict in local testing

@ca-scribner ca-scribner marked this pull request as ready for review February 16, 2024 16:17
@ca-scribner
Copy link
Contributor Author

PR is now ready for review, with proper tests. Any CI failures now are not intentional

Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

After a sync with @ca-scribner, we have agreed that the pattern that this rockcraft project follows must be documented in a CONTRIBUTING.md file so it is more clear how to deal with these kind of rocks, as some workarounds were put in place to make things work similarly to the Dockerfile it is based on.
I'll do another pass once we have documented everything that's needed (separate PR).

* Removes detailed comments (they have been moved to a CONTRIBUTING.md file which will be implemented separately)
* moves build-essential and libgomp1 to build-packages from overlay-packages to keep them from being put into the final rock
@ca-scribner ca-scribner enabled auto-merge (squash) February 20, 2024 18:00
Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

Thanks @ca-scribner. The contributing guide in #27 makes all the workarounds in this rock easier to understand, and after our discussion yesterday, I think we can carry on with this approach for similar kserve server rocks.

@ca-scribner ca-scribner merged commit 09a6562 into main Feb 21, 2024
7 checks passed
@ca-scribner ca-scribner deleted the KF-5326-sklearnserver-rock branch February 21, 2024 13:11
ca-scribner pushed a commit that referenced this pull request Feb 23, 2024
Refactors the pmmlserver rock to use the same patterns as #23
ca-scribner pushed a commit that referenced this pull request Feb 23, 2024
Refactors the paddleserver rock to use the same patterns as #23
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.

2 participants