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

Create Pip Package #21

Closed
wants to merge 13 commits into from
Closed

Conversation

abetlen
Copy link

@abetlen abetlen commented Apr 10, 2023

Hey @saharNooby here is some preliminary work to set up a python package similar to the one I've set up in llama-cpp-python using scikit-build to handle building the shared library. This is still a work-in-progress so I'm totally open to change anything / answer any questions / etc. Let me know what you think

Changes

  • Add scikit-build artifacts to gitignore
  • Add shared library target to Makefile
  • Add scikit-build specific section to CMakeLists
  • Add project scaffolding files (setup.py, pyproject.toml)
  • Update shared library name for macos (use Makefile instead of cmake to avoid M1 architecture issues)
  • Search for shared object file in package
  • I also created a package on PyPI just to test this out, if you have a username there I can add you as the project owner :)

@abetlen abetlen marked this pull request as draft April 10, 2023 07:41
@abetlen abetlen changed the title Create Python Package Create Pip Package Apr 10, 2023
CMakeLists.txt Outdated
@@ -245,3 +245,28 @@ if (BUILD_SHARED_LIBS)
set_target_properties(rwkv PROPERTIES POSITION_INDEPENDENT_CODE ON)
target_compile_definitions(rwkv PRIVATE RWKV_SHARED RWKV_BUILD)
endif()

if(SKBUILD)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if(SKBUILD)
if (SKBUILD)

Makefile Show resolved Hide resolved
setup.py Outdated
from skbuild import setup

setup(
name="rwkv_cpp_python",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess just rwkv_cpp or more preferred rwkv-cpp is fine, since this is already a Python package :)

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, I'll update this as well. I believe both alias to the same name on PyPI.

@saharNooby
Copy link
Collaborator

saharNooby commented Apr 10, 2023

Huge thanks! Sorry if it is inappropriate to leave comments on draft PR. Let me know when this is ready for me to check whether it builds!

BTW, do I need to register somewhere/do something extra to publish the package to pip?

@abetlen
Copy link
Author

abetlen commented Apr 10, 2023

Hure thanks! Sorry if it is inappropriate to leave comments on draft PR. Let me know when this is ready for me to check whether it builds!

Not at all, thank you so much for all your work on this project, hopefully this will let more people (like me) experiment with RWKV in their projects. And yes, I'll let you know once it's ready to test, the build currently works correctly but I'm having some pure python importing issues.

BTW, do I need to register somewhere/do something extra to publish the package to pip?

Yes, just head over to https://pypi.org/account/register/ and once you've registered let me know the username.

@saharNooby
Copy link
Collaborator

@abetlen Registered on PyPi: https://pypi.org/user/saharNooby/

@abetlen
Copy link
Author

abetlen commented Apr 10, 2023

@abetlen Registered on PyPi: https://pypi.org/user/saharNooby/

Awesome, invite sent.

@abetlen
Copy link
Author

abetlen commented Apr 11, 2023

Latest changes:

  • Had to rename rkwv folder to rwkv_cpp, unfortunately no way around this as python package import must have the same name as the folder they sit in and there's already an official non-cpu RWKV package so this avoids conflicts.
  • Removed the nested requirements file as this is already covered by the setup.py script

@saharNooby saharNooby mentioned this pull request Apr 18, 2023
@iacore
Copy link

iacore commented Apr 23, 2023

Is this branch stable? I'm going to back-port my changes onto this.

@abetlen
Copy link
Author

abetlen commented Apr 23, 2023

@iacore yes these are all my changes so far, really appreciate that! Sorry I meant to put more work in here but I got a little busy in the last week.

@saharNooby saharNooby mentioned this pull request Jun 1, 2023
@saharNooby saharNooby closed this Aug 20, 2023
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