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

Remove libprotobuf dep #1249

Merged
merged 26 commits into from
Nov 13, 2023
Merged

Conversation

jdye64
Copy link
Collaborator

@jdye64 jdye64 commented Oct 24, 2023

No description provided.

@jdye64 jdye64 marked this pull request as draft October 24, 2023 18:49
@jdye64 jdye64 self-assigned this Oct 25, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2023

Codecov Report

Merging #1249 (aed5f0f) into main (4f34d5f) will not change coverage.
The diff coverage is n/a.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##             main    #1249   +/-   ##
=======================================
  Coverage   85.55%   85.55%           
=======================================
  Files          77       77           
  Lines        4257     4257           
  Branches      758      758           
=======================================
  Hits         3642     3642           
  Misses        446      446           
  Partials      169      169           

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@jdye64
Copy link
Collaborator Author

jdye64 commented Oct 26, 2023

Do not merge until apache/datafusion-python#527 is merged and this PR is updated first.

@jdye64 jdye64 marked this pull request as ready for review October 26, 2023 18:29
@jdye64
Copy link
Collaborator Author

jdye64 commented Oct 27, 2023

rerun tests

@jdye64
Copy link
Collaborator Author

jdye64 commented Nov 7, 2023

@charlesbluca Any chance you could take a look at this today?

Copy link
Collaborator

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Thanks for this work @jdye64, this looks good so far. Looks like there's still some bits to the wheel release workflow that need to be modified to remove protoc installation, mind if I push some commits to get that working?

@@ -6,7 +6,7 @@ channels:
- nvidia
- nodefaults
dependencies:
- c-compiler
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on the conda recipe, seems like we still need a C compiler for compilation - do we need to modify this pinning in some way?

@@ -15,7 +15,7 @@ dependencies:
- intake>=0.6.0
- jsonschema
- lightgbm
- maturin>=1.1,<1.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just out of interest, what are the specific features we need from the newer version of maturin? Also might make sense to pin this a little more tightly if the specific version we need is 1.3.1:

Suggested change
- maturin>=1.1,<1.2
- maturin>=1.3.1,<1.4

@@ -1,4 +1,4 @@
rust_compiler_version:
- 1.69
- 1.73
maturin:
- 1.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should change this to match whatever pinned maturin version we converge on; assuming for now that's 1.3.1:

Suggested change
- 1.1
- 1.3.1

This was referenced Nov 7, 2023
@charlesbluca charlesbluca merged commit 807abee into dask-contrib:main Nov 13, 2023
31 checks passed
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.

None yet

3 participants