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

fix: exla rpath #1553

Merged
merged 7 commits into from
Oct 30, 2024
Merged

fix: exla rpath #1553

merged 7 commits into from
Oct 30, 2024

Conversation

polvalente
Copy link
Contributor

No description provided.

@polvalente polvalente self-assigned this Oct 30, 2024
exla/Makefile Outdated
ifeq ($(MIX_BUILD_EMBEDDED), true)
XLA_EXTENSION_LIB_RPATH = "xla_extension/lib"
else
XLA_EXTENSION_LIB_RPATH = "../xla_extension/lib"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

small fix needed due to the addition of the version to the EXLA lib compile path

@polvalente
Copy link
Contributor Author

There's some weird interplay between how EXLA looks up the rpath if it's the main app or if it's a library.
Just copying over the unversioned cache .so seems to be the simplest solution.

This probably doesn't solve the problem of dealing with a stale libexla.so, but the new env var, even in partial mode, will deal with rebuilding everything for a new version

exla/mix.exs Outdated
@@ -143,6 +143,9 @@ defmodule EXLA.MixProject do
"0" ->
:none

"false" ->
Copy link
Member

Choose a reason for hiding this comment

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

I would do this:

case System.get_env("EXLA_FORCE_REBUILD") do
  value when value in [nil, "0", "false"] ->
    :none

  "partial" ->
    :partial

  value when value in ["true", "1"] ->
    :full

  other ->
    ...
end

A cond also works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the readability is either equal or worse in that alternative, given it's a small set of possible values

Copy link
Member

Choose a reason for hiding this comment

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

With cond readability should be just as good as today and it should be easier to scan through. Your call :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed this would suit cond perfectly.

Copy link
Collaborator

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

There is an issue here in that the .so can be huge, back then it was 300MB, so duplicating that in disk can be too much.

@josevalim
Copy link
Collaborator

Maybe we keep the .o versioned and not the final one. This should be enough for make to trigger a rebuild whenever the version changes.

@polvalente
Copy link
Contributor Author

There is an issue here in that the .so can be huge, back then it was 300MB, so duplicating that in disk can be too much.
I checked and it was like 2MB or something.

I'll keep libexla.so unversioned for simplicity then.
The main problem was stale intermediate .o, and that's already solved

polvalente and others added 2 commits October 30, 2024 13:39
@polvalente polvalente merged commit 9e2cd04 into main Oct 30, 2024
8 checks passed
@polvalente polvalente deleted the pv-fix/exla-rpath branch October 30, 2024 16:53
josevalim added a commit that referenced this pull request Nov 16, 2024
Co-authored-by: José Valim <[email protected]>
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