Skip to content

magic underscore error messages #2072

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

Closed
nicolaskruchten opened this issue Jan 15, 2020 · 8 comments
Closed

magic underscore error messages #2072

nicolaskruchten opened this issue Jan 15, 2020 · 8 comments
Assignees
Milestone

Comments

@nicolaskruchten
Copy link
Contributor

When doing something like .update_layout(geo_lataxis_nonexistent=False) the validation error I get today is from go.Layout but it would be better to have it come from go.Layout.geo.lataxis or however far in the magic_underscore_path is correct

@nicolaskruchten nicolaskruchten added this to the v4.x milestone Jul 16, 2020
@nicholas-esterer nicholas-esterer self-assigned this Sep 23, 2020
@nicholas-esterer
Copy link
Contributor

When a property is set via an argument using the __init__ method, all the known arguments are processed first, then BasePlotlyType._process_kwargs in packages/python/plotly/plotly/basedatatypes.py is called on the arguments that didn't match any known arguments. This function uses the in keyword to see if the argument is valid, which calls BasePlotlyType.__contains__. This function actually goes down the tree of magic underscores and finds the last object that was found using a valid specifier. But when the function fails it simply returns False (which it probably needs to do in order to be compatible with in). So one way to do this would be to make __contains__ into a function that wraps another function called say walk_magic_underscore_tree. This way if __contains__ returns False, we can call walk_magic_underscore_tree on the argument it returned False on to see how far down the tree it got before failing. Of course it'd be "more optimal" to not call the same function twice, but I worry that changing the __contains__ function to something else that returns a boolean and the last object it found would require a lot of changes elsewhere in the code (we'd have to change in calls to something else expecting 2 return values).

@nicholas-esterer
Copy link
Contributor

Below I've tried different ways of using magic underscores to see where I need to tackle the problem and if I am missing any use cases Here it is clear the change needs to be made both in BaseFigure and BasePlotlyType, maybe there are other places I missed? Also are there ways to use magic underscores that I missed?

import plotly.graph_objects as go

fig=go.Figure()
fig.add_trace(go.Scatter(x=[],y=[]))

try:
    print(fig['layout_flakes'])
except KeyError as e:
    # This works properly but it could be improved by saying that it's trying to
    # assign a key called "flakes" in layout and failing.
    print("<<<Accessing property of figure using key")
    print(e.args)

try:
    fig['layout_flakes']=(go.layout.Shape(),)
except ValueError as e:
    # This works properly, maybe it could print where it got to before failing, e.g,:
    #
    # Bad Property Path: layout_flakes
    #                           ^
    # so users can see exactly where the problem is.
    print("<<<Setting property of figure using key")
    print(e.args)

try:
    fig2 = go.Figure(
	data=[go.Scatter(y=[1, 3, 2], line_color="crimson")],
	layout_title_txt="Another Graph Object Figure With Magic Underscore Notation"
    )
except TypeError as e:
    # This should instead say:
    # 'invalid layout.Title property "txt"'
    # and maybe where it got to in the path before failing
    print("<<<Setting property of figure with underscore constructor parameter")
    print(e.args)

try:
    fig3 = go.Figure(
	data=[go.Scatter(y=[1, 3, 2], line_colr="crimson")],
	layout_title_text="Another Graph Object Figure With Magic Underscore Notation"
    )
except ValueError as e:
    # The same as below: reports error for Scatter, not Line
    # This should say 'Invalid property specified for object of type
    # plotly.graph_objs.layout.shape._line.Line'
    print("<<<Setting property of Scatter with underscore constructor parameter")
    print(e.args)

try:
    print(fig.layout_flakes)
except AttributeError as e:
    # Not sure whether or not this should be supported because also
    # fig.layout_shapes doesn't work.
    print("<<<Accessing property of figure using field")
    print(e.args)

try:
    fig.add_shape(
        type='rect',
        x0=1,
        x1=2,
        y0=1,
        y1=2,
        line_colr='red'
    )
except ValueError as e:
    # For this it will report ValueError for Shape when we'd prefer one for Line
    # This should say 'Invalid property specified for object of type
    # plotly.graph_objs.layout.shape._line.Line'
    print("<<<Setting property of Scatter with underscore constructor parameter")
    print("<<<Function parameter using underscore path")
    print(e.args)

try:
    fig.add_shape(dict(
        type='rect',
        x0=1,
        x1=2,
        y0=1,
        y1=2,
        line_colr='red'
    ))
except ValueError as e:
    # For this it will report ValueError for Shape when we'd prefer one for Line
    # Seems almost exactly the same as using function underscore path parameter
    # This should say 'Invalid property specified for object of type
    # plotly.graph_objs.layout.shape._line.Line'
    print("<<<Setting property of Scatter with underscore constructor parameter")
    print("<<<Function parameter as dict containing underscore path")
    print(e.args)

try:
    shape=go.layout.Shape()
    shape['line_colr']='red'
except ValueError as e:
    # This one actually finds and reports ValueError for Line, maybe we'd want
    # the path that got us to Line though
    print("<<<Setting property of BasePlotlyType as using key")
    print(e.args)

try:
    shape=go.layout.Shape()
    print(shape['line_colr'])
except KeyError as e:
    # This one actually finds Line, but doesn't tell you the valid properties of Line
    print("<<<Accessing property of BasePlotlyType as using key")
    print(e.args)

@nicholas-esterer
Copy link
Contributor

Some more notes:
Adjusting BasePlotlyType.__contains__ to report a deeper error will make the error message better for when subclasses of this are constructed.
For underscore property assignment of BasePlotlyType, the non-scalar case needs to be adjusted in BasePlotlyType.__setitem__. The check made in BasePlotlyType.__contains__ that gives the deeper error could probably be reused here. The same for __getitem__
Similarly, Adjusting BaseFigure.__contains__ to report a deeper error will make the error message better for when subclasses of this are constructed, also __setitem__ and __getitem__ need to be updated like for BasePlotlyType to fix underscore joined property assignment.

@nicolaskruchten
Copy link
Contributor Author

Looks like your set above is pretty comprehensive. Indeed I don't think we need to catch situations like fig.layout_blah because in general magic underscores aren't supported there.

One additional note, however is that we also support stuff like fig["layout.showlegend"] = False (i.e. . instead of _ for dictionary keys) and ideally this mechanism would work here also :)

@nicholas-esterer
Copy link
Contributor

Right now (assuming we have a shape defined), calling fig['layout.shapes[0]'] returns:

layout.Shape({
    'type': 'rect', 'x0': 1, 'x1': 2, 'y0': 3, 'y1': 4
})

but fig['layout.shapes[0].x2000'] gives

KeyError: 'x2000'

However fig['layout.shapes[0].line_colr']="blue" or fig['layout.shapes[0].line.colr']="blue" gives

ValueError: Invalid property specified for object of type plotly.graph_objs.layout.shape.Line: 'colr'

    Valid properties:
        color
            Sets the line color.
        dash
            Sets the dash style of lines. Set to a dash type string
            ("solid", "dot", "dash", "longdash", "dashdot", or
            "longdashdot") or a dash length list in px (eg
            "5px,10px,2px,2px").
        width
            Sets the line width (in px).

Do we also want a similar error for the "x2000" case above where it lists the properties of shape? Or is it sufficient to just tack on a string to the error message like:

Bad property path:
layout.shapes[0].x2000
                 ^

@nicolaskruchten
Copy link
Contributor Author

When you say "Right now" you mean on master or in your branch?

@nicholas-esterer
Copy link
Contributor

on master

@nicolaskruchten
Copy link
Contributor Author

I think the current behaviour on master is OK for the string when reading:

Setting fig['layout.shapes[0].yo']="blue" gives a decent error message, but reading x = fig['layout.shapes[0].yo'] just gives a key error... seems OK to me. In the same way, reading x = fig['layout.shapes[0].line_colr'] just says "key error: colr. This is kind of analogous to what happens if you try x = fig['layout.shapes[0]'].yo: you just get a standard Python-level message and not a custom one listing all the values.

It'd be nice to get a full listing, but let's scope the current issue to just working with magic_underscores please. I'm intrigued that fig['layout.shapes[0].line_colr'] = "blue" on master already seems to do the deep message... can we easily reuse the same logic elsewhere?

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

No branches or pull requests

2 participants