-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
gr.DateTime component #8713
gr.DateTime component #8713
Conversation
🪼 branch checks and previews
Install Gradio from this PR pip install https://gradio-builds.s3.amazonaws.com/77e04759927a338c8d2d36958c6090866b19c3cd/gradio-4.37.2-py3-none-any.whl Install Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@77e04759927a338c8d2d36958c6090866b19c3cd#subdirectory=client/python" Install Gradio JS Client from this PR npm install https://gradio-builds.s3.amazonaws.com/77e04759927a338c8d2d36958c6090866b19c3cd/gradio-client-1.2.1.tgz |
🦄 change detectedThis Pull Request includes changes to the following packages.
With the following changelog entry.
|
if interactive is not None: | ||
warnings.warn( | ||
"The `interactive` parameter is deprecated and will be removed in a future version. " | ||
"The LinePlot component is always interactive." |
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 its always static, right? We use "interactive" to mean that a user can change the value of the component by interacting with it via the Gradio UI
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 suppose. Before this PR, interactive
for plots specifically meant that you could hover over point for extra info, it had nothing to do with the normal static vs interactive definition.
Very interesting PR @aliabid94. Functionality generally looks good, but my main concern is that this PR introduces some high-level abstractions that are quite rigid and may not be flexible enough for building apps generally with Gradio.
![]() This doesn't feel like a single component, but rather a collection of buttons, two calendar components, and some links. We don't even have a Calendar component, so it seems like quite a complex component to add all at once. Its also inflexible -- what if a user doesn't want to have the buttons, or wants to display the calendars stacked vertically instead of horizontally? That being said, I understand the value of having both of the Calendar components together, as it enables things like the quick links. I would suggest at least leaving the buttons out of this component -- they can be added by a user if they want to enable the undo feature or have a specific button to apply the changes. Instead of bind()-ing to the button, you could bind to any change of either of the calendars
|
We should definitely have a standalone calendar component, a gr.DateTime perhaps. I can add it in this PR. Then users can create custom logic for interacting with time as well, including some of the suggestions you made. However, it is no replacement for gr.TimeRange. Gradio would be no replacement for Grafana if you had to write the 50-100 additional lines of logic that TimeRange allows by customly combining gr.DateTime and gr.Button, The function |
Synced with @aliabid94 on this and this is where we ended up:
I agree that having a dedicated
However, we could not converge on the second concern:
I understand that this method makes creating time series dashboard a breeze. However, it feels out of place to me as a method of cc @freddyaboulton @hysts if you have any thoughts on a good API for the second concern |
In the spirit of experimentation, let's go ahead with |
Added a gr.DateTime as well. Addressed almost everything I think, I cannot theme the calendar popup because it is not part of the DOM, its part of the browser. I think that's ok. Ready for review. |
Will review now! In general though, if we want to experiment with API design, we should do so via custom components as opposed to merging directly into core. It lets us iterate way faster and we don't have to worry about backwards compatibility or waiting on a core release to get bug-fixes and small improvements out in the hands of users. I think this has worked well already in the past:
We can make some custom components more visible by displaying in the docs page/ new website section like "Gradio Dashboarding" |
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.
Left some small comments but functionally works great and the now
syntax is really slick. Love the e2e tests as well!
gradio/components/datetime_range.py
Outdated
|
||
return (parse_item(value[0]), parse_item(value[1])) | ||
|
||
def api_info(self) -> dict[str, Any]: |
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 give more detailed info in the view_api page -
{'properties': {'date_string': {'description': 'Date string in format YYYY-MM-DD HH:MM:SS',
'maxItems': 2,
'minItems': 2,
'prefixItems': [{'type': 'string'}, {'type': 'string'}],
'title': 'Date String',
'type': 'array'}},
'required': ['date_string'],
'title': 'DateTimeModel',
'type': 'object'}
else: | ||
return datetime.fromtimestamp(value).strftime(self.time_format) | ||
|
||
def api_info(self) -> dict[str, Any]: |
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.
Same comment here about the api info as datetime_range
+1 on the custom component route for experimentation and quicker iteration. In addition to @freddyaboulton's examples, we also do this with This could be a great way to get more visibility for custom components as well & we can add to core later, in 5.x if needed. |
Hmm ok I'll move out DateTimeRange to a custom component for now. I do think users see custom components as "less supported" which hurts their adoption - I've had this reaction when I've suggested users use Modal, which is custom. |
show_label: if True, will display label. If False, the copy button is hidden as well as well as the label. | ||
include_time: If True, the component will include time selection. If False, only date selection will be available. | ||
type: The type of the value. Can be "timestamp", "datetime", or "string". If "timestamp", the value will be a tuple of two numbers representing the start and end date in seconds since epoch. If "datetime", the value will be a tuple of two datetime objects. If "string", the value will be the date entered by the user. | ||
timezone: The timezone to use for timestamps, such as "US/Pacific" or "Europe/Paris". If None, the timezone will be the local timezone. |
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.
Local to the developer or to the user of the app?
For the website to build correctly with the docs for this component, you'll need to add a:
|
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.
LGTM @aliabid94! Just some minor comments above
Co-authored-by: Abubakar Abid <abubakar@huggingface.co>
…gradio into time_range_component
Create a gr.DateTime component, and adds selection range ability to plots, so that exporation of data, especially that of time based data, becomes much more natural.
NOTE: The actual part where the graph re-rerenders to focus on the selected time range is broken because ALL plots currently are broken when it comes to accepting any component updates. Therefore sending an update via
return gr.LinePlot(x_lim=[new_start_x, new_end_x])
does nothing. This will be fixed in a follow up PR that refactors all plot components to accept updates. Follow up PR will also have gr.TimeRange work with all plots (currently only works with LinePlots).See datetimes demo.