fix: make exla build resilient to stale upgrades #1548

polvalente · 2024-10-25T02:36:34Z

exla/mix.exs

exla/lib/exla/nif.ex

jonatanklosko · 2024-10-25T05:04:19Z

Looks good to me!

josevalim · 2024-10-25T05:10:05Z

exla/README.md

+
+We recommend following the steps below:
+
+  1. If the error appeared after upgrading EXLA, ensure that you have the proper dependency versions given by [XLA](https://github.com/elixir-nx/xla). Afterwards, compile with `mix compile --clean-libexla-cache --force` to clean up all cached files.


mix compile is not going to work when compiled as a dependency. We would need to introduce a separate mix compiler or a separate task.

It won’t work in the sense that it won’t invoke the code you added in the mix.exs file. mix compile is only about the local project. Maybe an env var with mix deps.compile exla will work.

@josevalim it may works as mix deps.clean exla --build --clean-libexla-cache?

err mix deps.compile exla --force --clean-libexla-cache

But yeah, env var sounds better either way.

I am not sure if we pass options forward in all of these cases. Although we should probably make mix deps.clean for dependencies extensible, so we actually invoke clean in the relevant dep.

josevalim · 2024-10-25T05:13:34Z

exla/mix.exs

-  defp cached_make(_) do
+  defp cached_make(args) do
+    {parsed, _args, _invalid} =
+      OptionParser.parse(args, strict: [clean_libexla_cache: :boolean, force: :boolean])


Suggestion to make this a single env var called EXLA_FORCE_REBUILD, as explained above.

Env var is probably the simpler alternative :)

exla/Makefile

josevalim · 2024-10-25T05:51:22Z

exla/mix.exs

@@ -150,14 +167,21 @@ defmodule EXLA.MixProject do
    cached_so = Path.join([xla_cache_dir(), "exla", cache_key, "libexla.so"])
    cached? = File.exists?(cached_so)


I think you can mostly remove the changes to this function and instead simply write this:

Suggested change

cached? = File.exists?(cached_so)

cached? = System.get_env("XLA_FORCE_REBUILD") == nil and File.exists?(cached_so)

This will be enough to force a rebuild and write the new build to cache.

@josevalim I changed to using env vars, but kept 2 separate ones because there are many instances in which I wanted to leverage the .o caching while getting rid of the libexla.so file and cache, as well as instances where I wanted to clean-slate everything.

My problem with the above is that, if something is not working, I need to set two env vars to clear everything. So why not a single env var with different values?

XLA_FORCE_REBUILD=1 - remove all

XLA_FORCE_REBUILD=partial - keep .o

Also, is there any situation where you don't want to cache files to be rewritten? I don't see a reason to skip that as part of these options?

Finally note that we use XLA env vars for pretty much everything, since we have XLA_CACHE_DIR, etc.

I'm ok with having special values for the env var and having a single one.

I disagree with using XLA as the prefix, though, as this doesn't have anything to do with :xla. XLA_CACHE_DIR being used in EXLA is just an artifact of "let's use the same subdir for both XLA and EXLA". The origin of the variable is in :xla.

Also, is there any situation where you don't want to cache files to be rewritten? I don't see a reason to skip that as part of these options?

As for this part, I don't think we are skipping the rewrite.

I refactored the cached? check so that there's less confusion about whether rewrites will happen.
cached? will now be set to false regardless of the file existing if force rebuild is set.

Co-authored-by: José Valim <[email protected]> Co-authored-by: Jonatan Kłosko <[email protected]>

…x/nx into pv-fix/exla-build-for-upgrading

polvalente · 2024-10-28T08:09:27Z

exla/test/exla/device_memory_sharing_test.exs

@@ -27,14 +27,13 @@ defmodule EXLA.DeviceMemorySharingTest do
  end

  @tag :cuda_required
-  test "ipc handles don't crash the runtime when :local mode is selected" do
-    assert {:error, ~c"Invalid pointer size for selected mode."} ==
+  test "invalid ipc handles don't crash the runtime" do


This test was breaking locally (I guess I hadn't run it on my desktop since the update 😛)

josevalim · 2024-10-28T09:59:22Z

exla/mix.exs

+          false
+
+        "0" ->
+          false
+
+        "partial" ->
+          :partial
+
+        "true" ->
+          true
+
+        "1" ->
+          true


Suggested change

false

"0" ->

false

"partial" ->

:partial

"true" ->

true

"1" ->

true

:none

"0" ->

:none

"partial" ->

:partial

"true" ->

:full

"1" ->

:full

This should force all the code below to be explicit :)

josevalim

One final nitpick and ship it!

Co-authored-by: José Valim <[email protected]> Co-authored-by: Jonatan Kłosko <[email protected]>

polvalente added 2 commits October 24, 2024 23:34

fix: make exla build resilient to stale upgrades

fae990e

docs: markdown syntax

02f5f59

polvalente requested review from jonatanklosko and josevalim and removed request for jonatanklosko October 25, 2024 02:36

jonatanklosko reviewed Oct 25, 2024

View reviewed changes

exla/mix.exs Outdated Show resolved Hide resolved

jonatanklosko reviewed Oct 25, 2024

View reviewed changes

exla/lib/exla/nif.ex Outdated Show resolved Hide resolved

josevalim reviewed Oct 25, 2024

View reviewed changes

exla/Makefile Outdated Show resolved Hide resolved

josevalim reviewed Oct 25, 2024

View reviewed changes

polvalente and others added 5 commits October 28, 2024 02:45

changes due to code review

d7fa1f9

docs: update readme

0c302eb

Apply suggestions from code review

3da43e9

Co-authored-by: José Valim <[email protected]> Co-authored-by: Jonatan Kłosko <[email protected]>

Merge branch 'pv-fix/exla-build-for-upgrading' of github.com:elixir-n…

538f303

…x/nx into pv-fix/exla-build-for-upgrading

refactor: single env var

2866799

polvalente commented Oct 28, 2024

View reviewed changes

fix: missing default value

ad725e4

josevalim reviewed Oct 28, 2024

View reviewed changes

josevalim approved these changes Oct 28, 2024

View reviewed changes

refactor: explicit modes

7483d63

polvalente merged commit 5eb444e into main Oct 28, 2024
8 checks passed

polvalente deleted the pv-fix/exla-build-for-upgrading branch October 28, 2024 20:50

josevalim added a commit that referenced this pull request Nov 16, 2024

fix: make exla build resilient to stale upgrades (#1548)

2f50714

Co-authored-by: José Valim <[email protected]> Co-authored-by: Jonatan Kłosko <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make exla build resilient to stale upgrades #1548

fix: make exla build resilient to stale upgrades #1548

polvalente commented Oct 25, 2024

jonatanklosko commented Oct 25, 2024

josevalim Oct 25, 2024

josevalim Oct 25, 2024

jonatanklosko Oct 25, 2024

jonatanklosko Oct 25, 2024

jonatanklosko Oct 25, 2024

josevalim Oct 25, 2024

josevalim Oct 25, 2024

polvalente Oct 25, 2024

josevalim Oct 25, 2024

josevalim Oct 25, 2024

polvalente Oct 28, 2024

josevalim Oct 28, 2024

polvalente Oct 28, 2024

polvalente Oct 28, 2024

polvalente Oct 28, 2024

josevalim Oct 28, 2024

josevalim left a comment


		We recommend following the steps below:

		1. If the error appeared after upgrading EXLA, ensure that you have the proper dependency versions given by [XLA](https://github.com/elixir-nx/xla). Afterwards, compile with `mix compile --clean-libexla-cache --force` to clean up all cached files.

		@@ -150,14 +167,21 @@ defmodule EXLA.MixProject do
		cached_so = Path.join([xla_cache_dir(), "exla", cache_key, "libexla.so"])
		cached? = File.exists?(cached_so)

	cached? = File.exists?(cached_so)
	cached? = System.get_env("XLA_FORCE_REBUILD") == nil and File.exists?(cached_so)

fix: make exla build resilient to stale upgrades #1548

fix: make exla build resilient to stale upgrades #1548

Conversation

polvalente commented Oct 25, 2024

jonatanklosko commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim left a comment

Choose a reason for hiding this comment