-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Animations/Frames in Python API #584
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
Conversation
|
fyi, this is my PR for issue 8129 So far, |
|
@theengineear Should I write tests now as I go for offline or just continue? |
|
We're in a bit of a time-crunch, and my bet is that writing tests for this will be difficult. I'd suggest moving on and getting it to work online next. Nice job so far, btw! //cc @chriddyp |
| {data}, | ||
| {layout}, | ||
| {config} | ||
| ).then(function () {add_frames}).then(function(){animate}) |
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 PR has been merged and is on plotly.js/master (unreleased, but coming very soon?) and makes the syntax somewhat simpler. I'd elaborate here, but the PR description explains it fairly clearly. Calling addFrames separately will remain perfectly fine, but ideally will become unnecessary.
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.
Okay, fantastic. I think Chris mentioned that I'd have to do some rewriting when that gets added.
I'll leave it for now, but does that mean frames is available as a "valid attribute" for Python?
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, should just be a simplification. Long story short, given data, layout, frames:
frames[i].datafollows the same validation asdataitselfframes[i].layoutfollows the same validation aslayout
I'm not 100% sure what that means for validation, but on a strictly abstract level, it's conceptually simple.
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.
Oh, sorry, unless you were referring to python arguments instead of data validation… That PR just changes the call signature. Not 100% sure the other concerns, but feel free to ask whatever questions you need and I'll do my best to get you what you need.
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, I'm referring to this py.iplot(fig) call where fig contains data, layout and frames. Right now, I'm still getting that:
'frames' is not allowed in 'figure'
Path To Error: ['frames']
Valid attributes for 'figure' at path [] under parents []:
['layout', 'data']
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, got it. Yeah, it should permit frames.
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 am still receiving the error. Anything I can do to fix? I sudo pip upgraded just now and it reported no new changes.
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.
Hmm… not sure. It seems like that's a whitelisting issue on the plotly (not js) side.
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.
Okay, so after playing follow-that-code for a while, I've traced it to the v2 plot schema Can we add frames to this?
|
Just threw in code from #586 without merging it formally |
… started work on generating grid/plot in v2 in create_animations
plotly/plotly/plotly.py
Outdated
| return r | ||
|
|
||
|
|
||
| def create_animations(fig, kwargs, payload): |
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.
Would appreciate some guidance...
I wrote something that can pretty reliably break apart a simple figure (2D Scatterplots for example) and make a grid then plot, and then a request to the V2 REST API. I't's working and I can view my stuff on the cloud.
I am getting an error when trying to pass frames into the figure for the plot generation: I just get some gargled HTML. What is the next step for me getting frames to pass?
A few more things I need to tackle:
-make sure I include things like sharing in the create_animations call.
-eventually, once frames is being allowed to exist in DATA, validate items in frames the same way I am here
Any thoughts/ideas are appreciated.
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 should not do figure splitting (i.e., exchanging raw data for referenced data). Please take my word on it. It's not worth it. It's extremely complicated (Please take my word on it).
^^ Seriously, take my word on it.
Users should be able to do something like:
grid = Grid()
grid.add_column('time_data', [1, 2, 3, 4]) # something like this...
grid.add_column('voltage', [120, 119, 122, 121]) # something like this...
# somehow, someway, add data to this grid and make it easy for users to pull out the reference.
# try looking in https://github.com/plotly/plotly.py/blob/master/plotly/grid_objs/grid_objs.py
figure.data[0].x = grid.time_data
figure.frames[0]['data'][0] = grid.time_data # Not sure if this is right, just for example
create_animations(figure, ...)
AFAIK, you don't need to worry too much about checking for raw data in the figure. The api will catch you and the request will just fail.
Let's fully shift the responsibility to the user. If we get into the game of guessing, we'll just end up guessing wrong and creating bugs.
|
So finally I have a
Additionally a GRID as well as a PLOT is made in the user's account each time Andrew, do you mind looking at my function and giving some PEP-8/style/structure/all-that-fun-stuff comments if you have any? :) |
| figure_class.__init__ = __init__ | ||
|
|
||
| def __setitem__(self, key, value, **kwargs): | ||
| if key == 'frames': |
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.
Let's add a comment about this one. Maybe tie it to a new issue to better-integrate frames into Figure? As always, pls put the issue numer (or link) in the TODO comment.
|
|
||
| def _get_valid_attributes(self): | ||
| super(figure_class, self)._get_valid_attributes() | ||
| if 'frames' not in self._valid_attributes: |
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.
Ditto hear, link it to that same issue ^^
| 'ErrorZ': {'object_name': 'error_z', 'base_type': dict}, | ||
| 'Figure': {'object_name': 'figure', 'base_type': dict}, | ||
| 'Font': {'object_name': 'font', 'base_type': dict}, | ||
| 'Frames': {'object_name': 'frames', 'base_type': dict}, |
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.
Is this necessary? Did I accidentally leave this in or something ;P?
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.
No, I added it in. I don't know if it's actually being used though.
| default_height, global_requirejs): | ||
|
|
||
| figure = tools.return_figure_from_figure_or_data(figure_or_data, validate) | ||
| # force no validation if frames is in the call |
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.
TODO this and link to some issue pls :)
plotly/plotly/plotly.py
Outdated
| def _send_to_plotly(figure, **plot_options): | ||
| """ | ||
| """ |
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.
This has been an empty comment for apparently one year. Should I attempt writing something in it and get it corrected?
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.
oop, sorry. Weird. I just saw the change and questioned it. sure 🔪 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.
Write one or 'cut 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.
cut it is fine :)
plotly/plotly/plotly.py
Outdated
|
|
||
| # add layout if not in fig | ||
| if 'layout' not in fig: | ||
| fig['layout'] = {} |
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.
Unnecessary comment and this can be a clean one-liner as you've done before.
plotly/plotly/plotly.py
Outdated
| fig['layout'] = {} | ||
|
|
||
| # make a copy of fig | ||
| fig_with_uids = copy.deepcopy(fig) |
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.
Another unnecessary comment. Just do this and the above layout thing in two lines.
plotly/plotly/plotly.py
Outdated
| # make a copy of fig | ||
| fig_with_uids = copy.deepcopy(fig) | ||
|
|
||
| # make grid |
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, on the other hand, is a nice little comment, imo.
plotly/plotly/plotly.py
Outdated
| cols_dict["{name}, {x_or_y}".format(name=trace['name'], | ||
| x_or_y=var)] = { | ||
| "data": list(trace[var]), "order": counter | ||
| } |
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 it's going to take up 4 lines anyhow, I'd suggest something like?
key = '{}, {}'.format(trace['name'], var)
data = list(trace[var])
cols[key] = {'data': data, 'order': counter}
plotly/plotly/plotly.py
Outdated
| return r | ||
|
|
||
|
|
||
| def create_animations(fig, kwargs, payload): |
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 should not do figure splitting (i.e., exchanging raw data for referenced data). Please take my word on it. It's not worth it. It's extremely complicated (Please take my word on it).
^^ Seriously, take my word on it.
Users should be able to do something like:
grid = Grid()
grid.add_column('time_data', [1, 2, 3, 4]) # something like this...
grid.add_column('voltage', [120, 119, 122, 121]) # something like this...
# somehow, someway, add data to this grid and make it easy for users to pull out the reference.
# try looking in https://github.com/plotly/plotly.py/blob/master/plotly/grid_objs/grid_objs.py
figure.data[0].x = grid.time_data
figure.frames[0]['data'][0] = grid.time_data # Not sure if this is right, just for example
create_animations(figure, ...)
AFAIK, you don't need to worry too much about checking for raw data in the figure. The api will catch you and the request will just fail.
Let's fully shift the responsibility to the user. If we get into the game of guessing, we'll just end up guessing wrong and creating bugs.
|
Here's the gameplan. I'll remake the According to @etpinard, animations aren't supported in @theengineear, would you know anything about the .json endpoint? This seems like a later-down-the-line task but right now, So a recap:
Please advise different approaches if you think of any |
plotly/offline/offline.py
Outdated
| def _plot_html(figure_or_data, config, validate, default_width, | ||
| default_height, global_requirejs): | ||
| # force no validation if frames is in the call | ||
| # TODO - add validation for frames in call - # |
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.
Thanks for linking issues ❤️! Looks like this one may have been missed though.
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.
LOL, whoops! That's #605
plotly/plotly/plotly.py
Outdated
| return r_dict | ||
|
|
||
|
|
||
| def get_uid_by_col_name(grid_url, col_name): |
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 grid_url arg should be a little more flexible. Look at https://github.com/plotly/plotly.py/blob/master/plotly/plotly/plotly.py#L318. I'm OK with this as is. But I could see it being frustrating to be able to reference files one way in one place and another way in another place.
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. I sort of imagined that this would have been part of our Grid class? More like:
1. Get `Grid` from `grid_url` (symmetric to `get_figure`), this is a network operation
2. Get `uid` from `Grid` (which is a simple, local operation)
…s to retrieve uid from column name
| default_height, global_requirejs): | ||
| # force no validation if frames is in the call | ||
| # TODO - add validation for frames in call - # | ||
| # TODO - add validation for frames in call - #605 |
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.
Fixed now 👍
|
@mdtusz - If validation is the only thing that's blocking, I'm think that it has been implemented in this branch already. You could work off of this branch in your environment with something like: |
And it has! |
|
@chriddyp @jackparmer @mdtusz @theengineear Okay, everything should be ready. Just need a review: Firstly, this is how a user can create a grid, retrieve the then grab a the You can find the result at the .json endpoint here A few notes about the PR:
|
|
Re
Does this PR also include a beta function that wraps this up nicely? From the previous discussion, I proposed something like:
|
Nope, but I'll write one up now! |
plotly/plotly/plotly.py
Outdated
|
|
||
| json = { | ||
| 'figure': figure, | ||
| 'world_readable': '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.
If we're JSON-ifying everything correctly in the request, then this can simply be True. You may need to add 'content-type': 'application/json' to the headers of the request
plotly/plotly/plotly.py
Outdated
| api_url = _api_v2.api_url('plots') | ||
| r = requests.post(api_url, auth=auth, headers=headers, json=json) | ||
|
|
||
| json_r = json.loads(r.text) |
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 can fail if the response wasn't JSON. we might want to do
try:
parsed_response = r.json()
except:
parsed_response = r.content
| json['world_readable'] = 'false' | ||
|
|
||
| api_url = _api_v2.api_url('plots') | ||
| r = requests.post(api_url, auth=auth, headers=headers, json=json) |
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 happens when the request fails? It can fail for a number of reasons:
- API key wasn't supplied
- API key wasn't correct
- User was offline
- Request was throttled
- Figure was malformed
- Duplicate filename
- Permission denied (can't save the file as
private) - Server was down
- And more
Errors can be detected by checking for non-200 level status codes or by calling r.raise_for_status(). Plotly returns the error in a standard format (something like {error: {message: 'error message'}} but I don't recall exactly) and we should parse the error message and then rethrow an error with that message for the user.
All of those errors should have a similar error response but you should try to hit those errors manually yourself to verify that the error handling works correctly and is helpful. Bonus points for including all of those types of errors inside actual tests so that we're sure that our error handling doesn't change without us knowing.
|
|
||
| # set filename if specified | ||
| if filename: | ||
| json['filename'] = filename |
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.
In py.plot, if the user specifies a / in the filename then we'll automatically make folders for them. That won't happen here. We might want to check for / in the filename and then issue a warning to the user that this beta function won't create folders automatically for them.
plotly/plotly/plotly.py
Outdated
| if sharing == 'public': | ||
| json['world_readable'] = 'true' | ||
| elif sharing == 'private': | ||
| json['world_readable'] = 'false' |
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 add 'secret' in here. we should also throw an error if sharing isn't one of public, private, or secret
plotly/plotly/plotly.py
Outdated
|
|
||
| def create_animations(figure, filename=None, sharing='public'): | ||
| """ | ||
| Put description 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.
⚡️
plotly/plotly/plotly.py
Outdated
| """ | ||
| Put description here. | ||
| For parameter descriptions, see the doc string for `py.plot()`. |
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.
instead of py.plot, plotly.plotly.plot since py is just an alias.
| For parameter descriptions, see the doc string for `py.plot()`. | ||
| `private` is not supported currently for param 'sharing'. Returns the | ||
| url for the plot if response is ok. | ||
| """ |
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 include some basic examples of animations in here.
|
|
||
|
|
||
| def create_animations(figure, filename=None, sharing='public'): | ||
| """ |
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 tell the user that this is a beta endpoint that is subject to deprecation in the future.
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 we should describe how this is similar and dissimilar to plotly.plotly.plot. Includes things like:
- Folders aren't supported
- Overwrite isn't supported
- Animations via frames are supported
- There might be some other things that I'm forgetting about.
plotly/plotly/plotly.py
Outdated
| return json_res | ||
|
|
||
|
|
||
| def create_animations(figure, filename=None, sharing='public'): |
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 might want to create an i version of this that works in ipython notebooks like icreate_animations or else add it as like an output method.
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.
👍
plotly/plotly/plotly.py
Outdated
| r = requests.post(api_url, auth=auth, headers=headers, json=json) | ||
|
|
||
| json_r = json.loads(r.text) | ||
| return json_r['file']['web_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.
I forget if this URL includes the title text at the end of the URL (e.g. https://plot.ly/~chris/15/my-title-of-my-graph) or not. We should make sure that we're returning the minimal version of the url like https://plot.ly/~chris/15 just like how py.plot does
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 it is minimal but will check
| json_r = json.loads(r.text) | ||
| return json_r['file']['web_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.
We should add tests for this function including the possible types of errors that can occur
theengineear
left a comment
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.
Couple requests (nothing too major). One thing that is still confusing me though:
How is a user supposed to populate *src fields with something like adam:10:aaaaaa?
plotly/grid_objs/grid_objs.py
Outdated
| "'username:187'." | ||
| ) | ||
| # TODO: verify that grid_id is a correct fid if a string | ||
| self.id = grid_id |
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 consistency, can we call this self.fid?
plotly/grid_objs/grid_objs.py
Outdated
| ``` | ||
| """ | ||
| def __init__(self, iterable_of_columns): | ||
| def __init__(self, columns_or_json, grid_id=None): |
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 consistency, can we call this fid?
plotly/grid_objs/grid_objs.py
Outdated
| if column.name == column_name: | ||
| return column | ||
|
|
||
| def get_fid_uid(self, column_name): |
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.
Why is it called get_fid_uid? Maybe just get_uid? Also, are users supposed to use this? They shouldn't need to know about uids or srcs. If it's something users are playing with, I'd suggest get_column_reference or something (which should return the fid:uid pair (and could potentially handle 2d references).
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, users are supposed to use this. And it does return the fid:uid pair
plotly/grid_objs/grid_objs.py
Outdated
| if column.name == column_name: | ||
| uid = column.id | ||
| break | ||
| return uid |
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 this raise if it can't find a uid? Is there a case where we would want to accept the empty string if a column uid cannot be found?
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 right, it should. It's possible for the column_reference to be the empty string '' if a grid hasn't been uploaded yet, for instance.
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.
grids are instantiated with ids so I don't think that's a risk here. For now, I think it's fine to have it just return the id of that column, whatever it happens to be.
plotly/plotly/plotly.py
Outdated
|
|
||
| def get_grid(grid_url, raw=False): | ||
| """ | ||
| Returns a JSON figure representation for the specified grid. |
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 feels a little misleading. Aren't we returning either JSON or a custom object. And, in the JSON case, it's a dict, right.
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.
Correct again! A oversight on my part.
| 'plotly-apikey': api_key, | ||
| 'plotly-version': version.__version__, | ||
| 'plotly-platform': 'python'} | ||
| upload_url = _api_v2.api_url('grids') |
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.
Did the _api_v2 thingy not have a get_headers method?
plotly/plotly/plotly.py
Outdated
| to `plotly.plotly.plot`, folder-creation and overwriting are not supported | ||
| but creating a plot with or without animations via frames is supported. | ||
| :param (str) filename: if set to 'None', an automatically generated plot |
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.
🐐 automatically-generated I think.
| username, api_key = credentials['username'], credentials['api_key'] | ||
| auth = HTTPBasicAuth(str(username), str(api_key)) | ||
| headers = {'Plotly-Client-Platform': 'python', | ||
| 'content-type': 'application/json'} |
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, I think we should have a get_headers function or something to reuse code a bit 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's this _api_v2.headers() which returns for me
{'authorization': u'Basic QWRhbUt1bGlkamlhbjpubHB2bTlhYWNr', 'plotly-client-platform': 'python 1.12.10'}
The thing is, usually I'm adding more things to headers so it wouldn't exactly make it more DRY ya know? @theengineear
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.
But you could improve the _api_v2.headers() method to allow for more things! 🐫! Centralizing functionality like this is almost always a good thing!
|
|
||
| json = { | ||
| 'figure': figure, | ||
| 'world_readable': 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.
Hm. @chriddyp not sure if you weighed in on this, but we typically default to world_readable: false, right?
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 that py.plot actually defaults to True. Moving forward, defaulting to False would probably be the right move.
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 double check the error message that gets returned when you hit this private file limit. We may want to modify the error message before we return it so that we can include helpful language like "Set "sharing" to "public" if you want to publish this graph with public access" or something (i.e. include the keyword "sharing" in the error message so that users know how to continue.)
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 is the private file limit? Is this for a particular type of subscription?
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 1 for Community users (similar to GitHub) - See https://plot.ly/products/cloud/
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 make set sharing to private as a default if the Community users get only 1. I think that may be confusing and more work if they have to set it to public each time they make a plot.
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.
An error message for hitting the private limit makes sense
theengineear
left a comment
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.
OK @Kully! I'm officially making these things non-blocking. I'd love it if you could make some changes based on my comments, but I'm ready to give this a 💃!
Nice job getting through this one! 💥 💣 ⚡️ . Let me know if you need a final round of review though, I'm always happy to do so.
plotly/grid_objs/grid_objs.py
Outdated
| if fid is None: | ||
| raise exceptions.PlotlyError( | ||
| "If you are manually converting a raw json/dict grid " | ||
| "into a Grid instance, you must ensure that make " |
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.
🐐 ensure that make
plotly/grid_objs/grid_objs.py
Outdated
| "'fid' is set to your file ID. This looks like " | ||
| "'username:187'." | ||
| ) | ||
| # TODO: verify that fid is a correct fid if a 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.
You mean validate it? It'd be a whole extra api call to actually check. I think you don't need to worry about it.
| "'username:187'." | ||
| ) | ||
| # TODO: verify that fid is a correct fid if a string | ||
| self.id = fid |
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.
🐄 non-blocking, but I'd go with self.fid 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.
Actually, if you call grid.id you'll get the fid back. That was in place before I started working on this PR. so id === fid for the grid.
plotly/grid_objs/grid_objs.py
Outdated
| ) | ||
| self._columns = ordered_columns | ||
|
|
||
| # fill in uids |
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.
🎎 uids --> column_ids.
|
|
||
| # fill in uids | ||
| for column in self: | ||
| column.id = self.id + ':' + columns_or_json['cols'][column.name]['uid'] |
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 typically use format i think. non-blocking
| raise exceptions.PlotlyError( | ||
| "Whoops, that column name doesn't match any of the column " | ||
| "names in your grid. You must pick from {cols}".format(col_names) | ||
| ) |
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.
👍
| username, api_key = credentials['username'], credentials['api_key'] | ||
| auth = HTTPBasicAuth(str(username), str(api_key)) | ||
| headers = {'Plotly-Client-Platform': 'python', | ||
| 'content-type': 'application/json'} |
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.
But you could improve the _api_v2.headers() method to allow for more things! 🐫! Centralizing functionality like this is almost always a good thing!
|
|
||
| try: | ||
| parsed_response = r.json() | ||
| except: |
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.
Any way to narrow this exception clause? It's too broad. If you just copied this code, I'm OK with leaving it as-is. Just sets off 🚨 when I see it.
plotly/plotly/plotly.py
Outdated
| parsed_response = r.content | ||
|
|
||
| if 'error' in r and r['error'] != '': | ||
| raise exceptions.PlotlyError(r['message']) |
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.
huh? Wouldn't this want to print out r['error'] If that's not the case can you add a quick code comment? This is a little confusing to me.
plotly/plotly/plotly.py
Outdated
| This function is based off `plotly.plotly.iplot`. See `plotly.plotly. | ||
| create_animations` Doc String for param descriptions. | ||
| """ | ||
| # TODO - create a wrapper for iplot and icreate_animations |
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.
Still need doing?
Relevant Issue: https://github.com/plotly/streambed/issues/8129