-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Dashboard wrapper #694
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
Dashboard wrapper #694
Conversation
Dashboard discussion thread: #646
A few notes:
-I need to add HTML preview representation. Current idea is to have a dashboard_html string that just represents the HTML for a rectangle. I want to add a couple methods that add lines/numbers to this representation and put them in the Dashboard class (it would just append to dashboard_html). Since we have to be able to pull dashboards from online, we would need to update the HTML preview in one-shot and not iteratively where it only updates each time you insert. Clearly if we are getting a JSON form of the dashboard online in one piece, we should be able to construct the HTML when we need it.
-hidden in the Dashboard dict is a box_ids_dict (I should probably rename to box_id_paths to be clear, but wanted to get this PR started 👍) and that is what keeps track of the number associated to each box. So a single item in there looks like {0: ['first', 'first', 'second']}.
-
strings docs aren't really finished. I'll worry about them once I know for sure I'm on the right track and get the thumbs up.
-
no tests for the same reason above.
-
need to add a few more manipulation methods (i.e. swap, rotate, resize). Shouldn't be too strenuous.
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.
Sweet! Excited to see some code for this! I've mostly made comments here to spark up discussion.
After seeing the implementation that generates all these new non-native objects, I'm thinking a simple wrapper around a completely native dict (self.content) would be a better approach.
This is definitely up for discussion. I just want to make sure we get the call signatures right and don't step on our own toes down the road!
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.
You should create/test a dashboards api file following the pattern of the other files in the v2 package.
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.
Since we're not doing validation, let's just have dashboard as the first argument and allow that to be a dict or a Dashboard object. This way, the api validation in the Plotly server can do the work for us.
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 is just a semantic thing right, since a dashboard is JSON and a Dashboard object is also JSON (a dict)? The renaming is for clarity in what can be passed, correct?
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.
I'm thinking of Dashboard being the class and a dashboard being either a *dictor aDashboard` here. So:
upload_dashboard(Dashboard(), ..)
OR
upload_dashboard({}, ..)
^^ is that clearer?
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.
And by class you mean the instance of a class correct? I think I get that, and 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.
The more I look at all these, the more I wonder whether we need non-native objects at all? We're just using these as dicts and the user isn't meant to interact with anything here...
What do you think of just having a top-level interface to an underlying dict? I.e., the Dashboard is just an interface to make it easier to manipulate a dashboard dict?
Initialization would look something like this:
class Dashboard(object):
def __init__(self, content=None):
if content is None:
content = {}
if not isinstance(content, dict):
raise TypeError('Content must be a dict.')
# Ensure top-level attributes exist.
content['version'] = 2
if content.get('settings') is None:
content['settings'] = {}
if content.get('layout') is None:
content['layout'] = {}
# Defer initialization of layout to general method.
self._initialize_split(content['layout'])
self.content = content
def _initialize_box(self, content):
# Ensure top-level attributes exist.
content['type'] = 'box'
# yadda, yadda, more logic to decide what *kind*
# of box this is and make sure to initialize it right.
def _initialize_split(self, content):
# Ensure top-level attributes exist.
content['type'] = 'split'
if content.get('first') is None:
content['first'] = {}
if content.get('second') is None:
content['second'] = {}
# yadda, yadda, more logic to decide what else we
# need to properly initialize the top-level attributes of
# the split
# Defer initialization to general methods.
if content['first'].get('type') == 'split':
self._initialize_split(content['first']):
else:
self._initialize_box(content['first'])
if content['second'].get('type') == 'split':
self._initialize_split(content['second']):
else:
self._initialize_box(content['second'])
Pros:
- No need to inherit from
dict, which means we have a handy separation-of-concerns. I.e., users create aDashboardinstance so that they can more-easily manipulate thecontent, which is a simpledict(no frills at all). - No confusing call signatures. The less-specific we are, the better here. We want as many details as possible to be handled by the backend API not by the api wrapper here.
- Only one new object for users to grok.
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.
^^ what do you think about this? Attempting to add too many frills has been a problem for us in the past. I don't want to repeat old mistakes if there's no reason to.
The important thing to note here is that the backend API should remain the ultimate source of truth. Dashboards are small JSON blobs; let's just throw whatever the user puts in there at our API and report back success/failure.
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.
I don't think it makes sense to ever cache this information. box_ids_dict should be calculated every time it is required (or... you'd need to figure out a smart way to do the caching...).
Consider the following steps:
- instantiate a dashboard (so you set
self.box_ids_dictduring instantiation) - manually change the dashboard in some way (which should be allowable for simplicity)
- hrm, was the
Dashboardinstance smart enough to update the box_ids? Yes? No? Shrug?
Imo, you should do the dumb thing (which also turns out to be the more-testable thing) and just calculate these box ids on the fly each time.
Thoughts?
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.
That's probably a better idea since that's exactly what I'm doing anyways when I pull a dashboard from online. I have code in here for dealing with that so reusing the same code would probably be best. So a big 👍 from me
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 is a private function, do we really need to protect it from bad call arguments? if you feel like it's important to protect it, that's OK, but it may not be necessary. Just a thought :)
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.
Yeah, just wanted to have it in there to show my current reasoning and to show what I want to prevent. Will have to look more closely and see if I can reorganize/remove it from the private function.
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.
🐄 I think path would be a better name than array_of_paths.
@theengineear @chriddyp @jackparmer
Alright. So here's what's going on.
-You can now use .insert(box, side=,box_id) on your dashboard and feed it a box you define (as a dict)
-the .get_preview() works well
-box_ids are written on the fly (much better idea)
-plotly.api.v2.dashboard.py was added to simplify uploading, retrieving, etc.
Need to add/improve:
-
dashboard_obj.upload() needs to be able to update existing filenames. I need to use
updateorpartial_updatewhich are already in v2 request functions, but it seemed to error out for me. Any ideas? -
the
sizeof each_container()is fixed at the display_height of the HTML representation (400 rn) but this needs to programatically change. In particular, if a user is making a dashboard on the fly, they don't wanna have to specify that your size is decreasing as you are shoving boxes into the layout. If you don't touch the size, the dashboard will have a bunch of plots that only display titles (they are collapsed). Need to implement a way of dealing with this. -
all doc strings should be properly written out
-
tests
Alright, let me know your thoughts and please pound me with as many questions as you want to ask!
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.
Awwwwwwwwwwwwwwwwwwwwwwwyeah! This is startin' to come together! Nice work on the HTML preview, I think it's worth the time! If that feels good for users, I think iterating on this will be pretty successful.
plotly/api/v2/dashboards.py
Outdated
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.
You're not using share_key, there are examples of how to do this in other api modules. (params)
plotly/api/v2/dashboards.py
Outdated
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.
I think you're probably aware, but this is not complete.
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.
How so? I briefly looked at https://api.plot.ly/v2/dashboards#update but yeah, I was going to attempt fixing the overwrite-filename limitation on upload, but just threw update in there to start
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.
😛 because the call signature doesn't give you any way to make an update.
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.
def update(fid, update)? or def update(fid, content), or something?
plotly/api/v2/dashboards.py
Outdated
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.
I think you're probably aware, but this is not complete.
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.
I.e., why can't we just do for node in node_generator(self['layout'])
I'll give this a try right now and see where we get... 👍
plotly/api/v2/dashboards.py
Outdated
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.
🐄 Use route, not id for this. It will do the same thing, except that it will read better here.
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 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.
🐄 Also, boolean * float? C'mon. ;)
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.
Wait, why not? Is it just a little clunky? I don't have to rewrite code and I like the simplicity.
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's pretty confusing to understand that, imo.
if is_horizontal:
new_box_w = box_w / 2
new_box_h = box_h
else:
new_box_h = box_w
new_box_h = box_h / 2
OR
new_box_w = box_w / 2 if is_horizontal else box_w
new_box_h = box_h if is_horizontal else box_h / 2
Pick either, but it's clearer to use a bool as a bool instead of implicitly as an int.
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.
Again, this is pretty confusing code.
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.
yeah, agreed, but I like the compactness. It reminded me of a simpler time...(math at uni)
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.
Yah, but when have you ever said to yourself:
wow, that was difficult to understand, I'm glad he didn't use another couple lines to make it easier to understand!
😸
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.
🎎 Pls use our standard docstring format for these args.
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.
haha, yeah I half assed it. I'll fix it all up
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.
🐫, I have a feeling you'll want to factor this out and generalize it.
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.
Shouldn't this initializer be based on the given box['type']?
I sorta stopped part-way through since I'm being pretty vocal. I don't want to be too overwhelming all-at-once :)
@Kully, okie dokie, responded. I'll be on this train for about 4 hours, after that, I probably will be hard to reach for a while 😄.
@Kully, okie dokie, responded. I'll be on this train for about 4 hours, after that, I probably will be hard to reach for a while 😄.
thanks buddy. I'll be sure to review.
@theengineear @jackparmer @chriddyp
A note about sizing of the containers/splits. I think we should hold off on giving the users' the ability to change the size for this first pass. I think it's something that they can toy around with in the online editor when they upload it. I was thinking about something like resize(box_id, percentage=60) where percentage will determine how much of the container the box corresponding to the box_id will take up, but even that's a little difficult to understand, since they would need to know about the direction of the particular container they are manipulating.
I think for now, I'm just going to make sure that the size of each container is programmatically set to values that match to the HTML rep. Thoughts?
NB The way I'm programming it now is that there is something like a _set_container_sizes() function that does all the magic. A potential issue is that a users' dashboard sizes will get erased and overwritten when they import using the dashboard wrapper. In spite of this, I'm still in favour of sticking with my idea above for now.
Hey @Kully - To make sure I'm understanding correctly, would layouts like this still be possible?
https://files.slack.com/files-pri/T06LPNGUD-F4B1SGXPC/screen_shot_2017年02月27日_at_1.26.25_pm.png
How would the widths of the cells in this HTML preview be set?
How would the widths of the cells in this HTML preview be set?
Yes they would. The way I'm doing it now is that I'm checking how "deep" each container is, meaning how long the path is in the dashboard, and then assigning it a size which decreases as the path length increases. The equation looks like MASTER_SIZE / (2^path_len)
I'm going to test my algo once Plotly is up and working.
Nice. The above sounds OK to me. We can revisit later if exact width sizing becomes a big issue.
@theengineear @chriddyp If one of you want to take a final 👀 for this PR.
Note: I still need to update CHANGELOG
taking a first peek at this now
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 might be nice to include a simple example of usage here
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'd be nice to include one or two simple getting started examples.
Ideally, the user has enough info when calling help() on all of these methods to get started. Here's what help looks like now:
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.
should we include a
from dashboard_objs import dashboard_objs
inside __init__.py that way you just have to do:
plotly.dashboard_objs.Dashboard
instead of
plotly.dashboard_objs.dashboard_objs.Dashboard
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 docstring with a simple example here would be great. That way, a user can just call help on this object and know what to do:
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.
similarly here - a couple simple examples of usage go a long way
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.
and here!
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.
here too :)
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 simple example here too would be pretty helpful
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.
For some reason these docstrings don't appear in help
image
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.
help(py.meta_ops.upload) also results in the same message.
I think it's fine since help(py.dashboard_ops) will display the doc strings for all the other methods in the class
plotly/plotly/plotly.py
Outdated
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.
"If 'private'", not "If 'secret'"
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 should also append the share key when the user sets the sharing to secret. Otherwise, they will probably mistakeningly try to share the URL to a coworker who won't be able to see it
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's taken care of here:
if sharing == 'secret':
url = add_share_key_to_url(url)
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.
Got it. It looks like the URL gets returned with the sharekey but the browser doesn't open up with the sharekey:
shareky
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.
NVM, just saw that you updated this logic in 9c4b235#diff-07784ffd70c058caed46ede342e6dc61R1459
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.
missing an else here.
I'm assuming that v2.dashboards.update will fail if the filename already exists as a different type like a plot?
Perhaps we can just throw exceptions.PlotlyRequestError('{filename} is already a {filetype} in your account. While you can overwrite dashboards with the same name, you can't change overwrite files with a different type.\nTry deleting '{filename}' in your account or changing the filename'.format(filename=filename, filetype=matching_dashboard['filetype']')
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.
Looking good to me! I was able to create a few different dashboards. Great stuff!
I just want to see some more docstrings to help newcomers out.
Looking good to me! I was able to create a few different dashboards. Great stuff!
I just want to see some more docstrings to help newcomers out.
Alrighty, all done! Waiting for tests to pass, then would love to merge!
@chriddyp Can I get a 💃 ?
💃
No description provided.