Skip to content

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
"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
Co-authored-by: José Valim <jose.valim@dashbit.co>
Co-authored-by: José Valim <jose.valim@dashbit.co>
@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 <jose.valim@dashbit.co>
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.

4 participants