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

Update CI and build process #20

Merged
merged 3 commits into from
Sep 10, 2024
Merged

Update CI and build process #20

merged 3 commits into from
Sep 10, 2024

Conversation

ErikKaum
Copy link
Collaborator

@ErikKaum ErikKaum commented Aug 30, 2024

Resolves: #18 and #21

Tasks for the python package:

  • cross compile to native lib to the architectures we want to support
  • add tests in CI for all cross compiled architectures (the tests is to import the package and print version)
  • add type info for the functions from the rust extension
  • update the pypi release CI script accordingly

Tasks for the rust crate:

  • CI script for publishing on crates.io
  • add documentation & publish to docs.rs, deferred**
  • (maybe) add tests to the rust side*

*we probably want to start by duplicating the tests from python to rust --> run them in parallel for a while --> once we're confident enough we can then start deleting some of the duplicate tests.

Supported python versions and architectures in the PR:

  • python versions: 3.7 3.8 3.9 3.10 3.11 3.12 (only CPython)
  • archs:
    • Linux: x86_64, i686 and aarch64
    • Windows: AMD64 and x86
    • MacOs x86_64 and arm64

notably musllinux is not on the list, maybe we want to support it?
Edit: yes, the alpine linux (docker image) uses musl libc, so it makes sense supporting it.

**Edit: I'll defer any more in depth rust code documentation. The code itself is quite self documenting when published to crates & docs. More detailed stuff is probably better done once we have a more clear understanding what the API will look like in the long run.

Additional question: A lot of the CI workflow are such that they cannot be tested without actually making a release. So it's quite likely that they have some bugs. I'd recommend that we do a pre-release. Something like version 0.1.0-dev0 to make sure everything runs smoothly. The requirement would thus be that we are comfortable in making this work somewhat public. Would that be okay for you?

brandonwillard
brandonwillard previously approved these changes Sep 4, 2024
python/outlines_core/fsm/outlines_core_rs.pyi Outdated Show resolved Hide resolved
@ErikKaum
Copy link
Collaborator Author

ErikKaum commented Sep 5, 2024

Btw if everything's cool could you @brandonwillard merge it? I don't seem to have the permissions.

Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

Looks good! I set up the tokens and tried the crates.io publish step locally, so it should work; however, I noticed that we might want to change the crate name to simply outlines-core and we might also need to explicitly list the license type.

@ErikKaum
Copy link
Collaborator Author

ErikKaum commented Sep 6, 2024

Looks good! I set up the tokens and tried the crates.io publish step locally, so it should work; however, I noticed that we might want to change the crate name to simply outlines-core and we might also need to explicitly list the license type.

Thank you 👍
Great! That's a good point, I changed of the package & lib the name in the Cargo.toml file. It still is imported to the python side as outlines_core_rs configuration in setup.py, which is probably good to avoid name collisions. Also added the Apache-2.0 as the license in Cargo.toml

Copy link
Contributor

@umut-sahin umut-sahin left a comment

Choose a reason for hiding this comment

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

We don't need to do anything for publish to docs.rs.

docs.rs automatically publishes all crates uploaded to crates.io (see https://docs.rs/releases/queue).

.github/workflows/tests.yml Outdated Show resolved Hide resolved
brandonwillard
brandonwillard previously approved these changes Sep 6, 2024
@ErikKaum
Copy link
Collaborator Author

ErikKaum commented Sep 9, 2024

Regarding the failing test: this seems to fail on main as well. Not sure if something was updated in the NousResearch/Hermes-2-Pro-Llama-3-8B tokenizer which uncovered something new in our test and therefore broke them. Or if it changed in a way that just breaks the contract.

Regardless, maybe we shouldn't block this PR on that test failing, but rather have a separate issue for it?

@brandonwillard brandonwillard merged commit 76211f7 into main Sep 10, 2024
6 of 7 checks passed
@brandonwillard brandonwillard deleted the update_ci branch September 10, 2024 16:27
@brandonwillard
Copy link
Member

Regardless, maybe we shouldn't block this PR on that test failing, but rather have a separate issue for it?

Yeah, it looks like we have some rebasing to do.

@ErikKaum
Copy link
Collaborator Author

Indeed! Thank you for merging 👍

@rlouf rlouf mentioned this pull request Oct 9, 2024
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.

Update build, CI and release scripts
3 participants