Skip to content

Conversation

@tokoko
Copy link
Contributor

@tokoko tokoko commented Oct 17, 2025

switches to using uv lockfile to run tests to make ci environment more reproducible.

@nielspardon
Copy link
Member

@tokoko what do you think about updating the https://github.com/substrait-io/substrait-python/blob/main/CONTRIBUTING.md file? that feels a bit out of sync to me if we are using uv I guess we don't need to tell people to use conda or am I missing something?

@tokoko
Copy link
Contributor Author

tokoko commented Oct 20, 2025

I guess we can do that, yeah. I've personally never used that conda environment, I'm fairly sure no one else is using it either.

@mbwhite
Copy link
Contributor

mbwhite commented Oct 21, 2025

@tokoko @nielspardon using uv would be a good idea I think for dependencies as well. To get a working local environment I was able to use uv successfully.

Happy to help here - testing or updating..

@nielspardon
Copy link
Member

nielspardon commented Oct 21, 2025

it looks like the issue is caused by a protobuf version incompatibility. the google.protobuf.any introduced by @mbwhite seems to only exist in more recent versions of protobuf but not in version 3.20.1 which uv seems to pick currently.

the older version only has google.protobuf.any_pb2

@mbwhite
Copy link
Contributor

mbwhite commented Oct 21, 2025

Checked what I've got locally in uv.lock and that is protobuf 3.20.1 - and the tests pass. And the build originally past; the virtual environments are very confusing.

Does the pyproject.toml need to have the dependencies added?

Update: Cloning this PR and running (python3.12) there aren't any issues with the tests... 🤷

It's the generation of the protos.. ah!

@nielspardon
Copy link
Member

I have a fix. will provide a separate PR

@nielspardon
Copy link
Member

this should fix it: #111

@tokoko
Copy link
Contributor Author

tokoko commented Oct 21, 2025

I fixed the tests in here as well. We can avoid bumping lower bound unnecessarily, especially when it's just test code.

@nielspardon
Copy link
Member

I fixed the tests in here as well. We can avoid bumping lower bound unnecessarily, especially when it's just test code.

great, that should also work

Copy link
Member

@nielspardon nielspardon left a comment

Choose a reason for hiding this comment

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

LGTM, just two typos I found in the markdown file

@nielspardon nielspardon merged commit 16eb837 into substrait-io:main Oct 21, 2025
18 checks passed
@tokoko tokoko deleted the uv-tests branch October 21, 2025 17:01
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