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

eigen: add version cci. 20240315 (pre 3.4.1) #25048

Merged
merged 3 commits into from
Sep 5, 2024

Conversation

mayeut
Copy link
Contributor

@mayeut mayeut commented Aug 26, 2024

Summary

Changes to recipe: eigen/all

Motivation

Add a cci specific version in order to be able to build #25039
The 3.4.0 version is 3 years old but unfortunately, there are no tags with all the fixes needed yet.

Details

https://gitlab.com/libeigen/eigen/-/compare/3.4.0...9df21dc8b4b576a7aa5c0094daa8d7e8b8be60f0?from_project_id=15462818&straight=false


@conan-center-bot

This comment has been minimized.

@valgur
Copy link
Contributor

valgur commented Aug 26, 2024

Use 3.4.0.cci.20240315 as the version name instead. The naming scheme for unofficial releases on CCI was recently updated by the team: #24769 (comment)

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 3 (6ee62c6cbe005134253a7953ae94cbc72c7a039f):

  • eigen/3.4.0.cci.20240315:
    All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 3 (6ee62c6cbe005134253a7953ae94cbc72c7a039f):

  • eigen/3.4.0.cci.20240315:
    All packages built successfully! (All logs)

@valgur
Copy link
Contributor

valgur commented Aug 26, 2024

LGTM, but why that specific version instead of the latest commit, though? onnxruntime currently uses a much older version altogether: https://gitlab.com/libeigen/eigen/-/commit/e7248b26a1ed53fa030c5c459f7ea095dfd276ac

@mayeut
Copy link
Contributor Author

mayeut commented Aug 26, 2024

but why that specific version instead of the latest commit, though?

This is the latest commit on 3.4 branch: https://gitlab.com/libeigen/eigen/-/tree/3.4?ref_type=heads
onnxruntime uses a commit on that branch > 3.4.0
It's expected to become 3.4.1 in the coming months.

@ErniGH ErniGH self-assigned this Aug 26, 2024
@jcar87 jcar87 self-assigned this Aug 26, 2024
@AbrilRBS
Copy link
Member

Hi @mayeut thanks a lot for the PR!

It's expected to become 3.4.1 in the coming months.

You mention that they might release the new patch version in the near future, so I wonder if it's better to wait for an official release/ask upstream to prioritize creating it. Do you have any insight into the timeline for 3.4.1?

@mayeut
Copy link
Contributor Author

mayeut commented Aug 27, 2024

@AbrilRBS, It's been asked a couple times already:

The answer is when they have time in the coming months.
I'd rather not have this block an onnxruntime update.

@valgur
Copy link
Contributor

valgur commented Aug 27, 2024

I agree with @mayeut. Besides onnxruntime, I have depended on a pre-release version of Eigen in all CUDA projects that make use of it, since it gets rid of a deluge of irrelevant nvcc warnings that the stable version otherwise produces.

Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

After discussing with @czoido we're moving ahead with this, thanks a lot for the extra info :)

@AbrilRBS AbrilRBS assigned czoido and AbrilRBS and unassigned ErniGH and jcar87 Sep 5, 2024
@conan-center-bot conan-center-bot merged commit 9a16dcf into conan-io:master Sep 5, 2024
12 checks passed
AbrilRBS added a commit that referenced this pull request Sep 5, 2024
jcar87 pushed a commit that referenced this pull request Sep 5, 2024
@jcar87
Copy link
Contributor

jcar87 commented Sep 5, 2024

Reverted here as the team reviews how to best proceed #25150

  • can't see any evidence of other package repositories publishing a pre-release of eigen outright
  • using this new version in onnxruntime, is likely to cause conflicts if users also depend on anything else that depends on eigen (which would be 3.4.0). causing them to override it to the newer version, exposes them to 3 years of commits of eigen and would cause other libraries to use an untested version
  • from past experience, eigen pre-releases may not be API stable, or be compatible once there is a release. especially the "unsupported" section, which onnxruntime relies on

We see risk in making an arbitrary commit available from a pre-release, and exposing it to users who either use version ranges (breaking a promise that it should resolve to release), or users who are force to override because to fix a potential conflict - eigen tends to be backwards compatible, there really isn't a guarantee that things would work, especially for any downstream dependency that uses things un unsupported.

I would try to narrow down exactly which patches of eigen are needed, and we can consider a cci version that is more minimal and minimises risks.

Additionally - it would be good to understand the urgency of having onnxruntime 1.19.0 available.

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.

7 participants