-
Notifications
You must be signed in to change notification settings - Fork 125
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 inspect between nodes #784
Fix inspect between nodes #784
Conversation
I had a problem when I tried to run the tests with the "mix ci" command... 6 tests are breaking because of this error:
I think I had this problem before and I had already solved it, but now that it's back I don't remember what I did... I've already tried to recompile, update the project, etc. All tests work with the "mix test --only cloud_integration" command. Versions I'm using:
I already tried with (which was the version I was on before):
Would anyone know why? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor detail :)
Would anyone know why? (About the Adbc exception)
Sorry, I don't remember either. But maybe José can help.
lib/explorer/data_frame.ex
Outdated
@@ -5785,6 +5785,12 @@ defmodule Explorer.DataFrame do | |||
defimpl Inspect do | |||
import Inspect.Algebra | |||
|
|||
def inspect(df, _opts) when node(df.data.resource) != node() do | |||
raise RuntimeError.exception( | |||
"It is not possible to inspect a DataFrame that belongs to another node" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We normally don't capitalize the message:
"It is not possible to inspect a DataFrame that belongs to another node" | |
"it is not possible to inspect a DataFrame that belongs to another node" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two issues here:
-
We probably should not raise, otherwise it makes impossible to see the value in logs, IEx, etc
-
df.data.resource
is accessing an implementation detail of the backend. Ideally we want to move this toExplorer.DF.PolarsBackend
or similar. And instead of raising, maybe we just print something like:
#Explorer.DataFrame<
Polars[node: other@foobar]
>
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to add a little bit: we have the column names and types. We could also display them if we want (without the values). WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's good. We can show them as:
Polars[node: ...]
foo s64 ???
bar string ???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/explorer/series.ex
Outdated
@@ -6048,6 +6048,12 @@ defmodule Explorer.Series do | |||
defimpl Inspect do | |||
import Inspect.Algebra | |||
|
|||
def inspect(series, _opts) when node(series.data.resource) != node() do | |||
raise RuntimeError.exception( | |||
"It is not possible to inspect a Series that belongs to another node" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"It is not possible to inspect a Series that belongs to another node" | |
"it is not possible to inspect a Series that belongs to another node" |
@@ -858,6 +858,10 @@ defmodule Explorer.PolarsBackend.DataFrame do | |||
# Inspect | |||
|
|||
@impl true | |||
def inspect(df, opts) when node(df.data.resource) != node() do | |||
Explorer.Backend.DataFrame.inspect(df, "Polars", nil, opts, from_another_node: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explorer.Backend.DataFrame.inspect(df, "Polars", nil, opts, from_another_node: true) | |
Explorer.Backend.DataFrame.inspect(df, "Polars", "node: #{df.data.resource}", opts, elide_columns: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will generate this error:
iex(test_0@myhost)1> df = Explorer.Datasets.fossil_fuels()
#Explorer.DataFrame<
Polars[1094 x 10]
year integer [2010, 2010, 2010, 2010, 2010, ...]
country string ["AFGHANISTAN", "ALBANIA", "ALGERIA", "ANDORRA", "ANGOLA", ...]
total integer [2308, 1254, 32500, 141, 7924, ...]
solid_fuel integer [627, 117, 332, 0, 0, ...]
liquid_fuel integer [1601, 953, 12381, 141, 3649, ...]
gas_fuel integer [74, 7, 14565, 0, 374, ...]
cement integer [5, 177, 2598, 0, 204, ...]
gas_flaring integer [0, 0, 2623, 0, 3697, ...]
per_capita f64 [0.08, 0.43, 0.9, 1.68, 0.37, ...]
bunker_fuels integer [9, 7, 663, 0, 321, ...]
>
iex(test_0@myhost)2> "node: #{df.data.resource}"
** (Protocol.UndefinedError) protocol String.Chars not implemented for #Reference<0.641693014.1003356202.157361> of type Reference. This protocol is implemented for the following type(s): Atom, BitString, Complex, Date, DateTime, Explorer.Duration, Float, Hex.Solver.Assignment, Hex.Solver.Constraints.Empty, Hex.Solver.Constraints.Range, Hex.Solver.Constraints.Union, Hex.Solver.Incompatibility, Hex.Solver.PackageRange, Hex.Solver.Term, Integer, List, NaiveDateTime, Time, URI, Version, Version.Requirement
(elixir 1.14.0) lib/string/chars.ex:3: String.Chars.impl_for!/1
(elixir 1.14.0) lib/string/chars.ex:22: String.Chars.to_string/1
iex:2: (file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, we need to inspect the node, but the overall idea should be solid. :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6511685
to
1247cf1
Compare
💚 💙 💜 💛 ❤️ |
This PR resolves #768
After this PR:
With DataFrame
With Series