elixir-nx / xla

Pre-compiled XLA extension
Apache License 2.0
83 stars 21 forks source link

Handle local archives without requiring network tooling #65

Open lexmag opened 6 months ago

josevalim commented 6 months ago

I am not a big fan of this approach. The current implementation is too complex by trying to support three different formats and it does not work on Windows. Perhaps it would make more sense to introduce another environment variable, instead of overloading this one?

jonatanklosko commented 6 months ago

@josevalim I just thought the same, we can have XLA_ARCHIVE_PATH. And we don't have to copy it to the cache dir, since the file is already accessible locally!

lexmag commented 6 months ago

I'm personally also not that positive about supporting file: all the way. That's said, I think if we decide to go just with a single format (for example, empty host), it is justifiable and does not seem worse than adding an extra environment variable (which, in turn, brings a question of priority).

jonatanklosko commented 6 months ago

@lexmag there's still the advantage of not creating a copy of the file at all. And having an env var for path makes it much more intuitive/discoverable. I don't think prioritisation is an issue, we can check the path before url, similarly to how we respect XLA_BUILD=true first. So the path env var check would go between these:

https://github.com/elixir-nx/xla/blob/137ca696e13e66af34d45bdb77704e3de0a3d1cd/lib/xla.ex#L22-L29