Skip to content
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

Experimental_memo does not support pytz-localized datetime arguments #5110

Closed
amhrasmussen opened this issue Aug 8, 2022 · 10 comments
Closed
Assignees
Labels
feature:cache Related to st.cache_data and st.cache_resource priority:P3 status:confirmed Bug has been confirmed by the Streamlit team type:bug Something isn't working

Comments

@amhrasmussen
Copy link
Contributor

amhrasmussen commented Aug 8, 2022

Summary

Using experimental_memo() on function with pytz-localized datetime arguments fails

Steps to reproduce

Open in Streamlit Cloud

Code snippet:

Run this app with streamlit:

import datetime
import pytz
import streamlit as st

@st.experimental_memo
def f(x: datetime.datetime):
    return x

t = datetime.datetime.now()
t_pytz = pytz.timezone("UTC").localize(t)

# This works
st.write(f(t))

# This raises UnhashableParamError
st.write(f(t_pytz))

Expected behavior:

Expect to see two dates (t and t_pytz) printed.

Actual behavior:

Only t is printed, t_pytz causes UnhashableParamError to be raised.

Is this a regression?

This used to work with cache()

Debug info

  • Streamlit version: 1.11.0
  • Python version: 3.10.5
  • Using PipEnv
  • OS version: WSL2, ubuntu 18.04
  • Browser version: Firefox 102.0.1

Additional information

Discussed with @kajarenc in #4876 (comment)

@amhrasmussen amhrasmussen added type:bug Something isn't working status:needs-triage Has not been triaged by the Streamlit team labels Aug 8, 2022
@kajarenc kajarenc self-assigned this Aug 8, 2022
@LukasMasuch LukasMasuch added the feature:cache Related to st.cache_data and st.cache_resource label Aug 22, 2022
@LukasMasuch
Copy link
Collaborator

LukasMasuch commented Oct 11, 2022

@amhrasmussen Thanks for reporting this issue! I was able to reproduce this here. It seems that we will need a custom hashing implementation for pytz-localized datetime objects.

@LukasMasuch LukasMasuch added status:confirmed Bug has been confirmed by the Streamlit team priority:P3 and removed status:needs-triage Has not been triaged by the Streamlit team labels Oct 11, 2022
@kajarenc
Copy link
Collaborator

kajarenc commented Jan 3, 2023

I remember we discussed this question a long time ago with @tconkling (which primitives should be hashable out of the box), and then if I remember correctly, we came to the conclusion that we will not implement all possible options, but we will probably do this for some of the most popular types).

I think we could look into this issue and have a (product?) decision for this as part of the de-experimentation of caching primitives.

@amhrasmussen as a workaround you can prefix your argument x with an underscore, so naming it _x inside a function will skip hashing step.

@th0ger
Copy link

th0ger commented Mar 24, 2023

@kajarenc Same issue here.
Prefixing _ is not really a "workaround", if the use case depends on datetime caching. It is equivalent to just remove all caching support in my app.
So what is the real workaround?

@kajarenc
Copy link
Collaborator

Hey, @th0ger (CC: @amhrasmussen ) we currently try to solve this issue and collecting feedback here #6295
Feel free to leave there your suggestions!

As for a potential workaround, if you need to use a pytz timezone-aware datetime in your function, it could be solved by just passing a datetime and a timezone string to the function, and constructing final object there, this should work as expected:

import datetime
import pytz
import streamlit as st


@st.cache_data(persist=True)
def f(t: datetime.datetime, timezone_str: str = "UTC"):
    x = pytz.timezone(timezone_str).localize(t)
    return x


current_time = datetime.datetime.now()

st.write(f(current_time))
st.write(f(current_time, 'America/New_York'))

@kevinddnj
Copy link

Sure that can workaround it, but it is messy in general - especially for a large multi-page application that uses timezone-aware datetimes extensively for filtering data. In my case it is Pandas Timestamps that the new caching primitives don't like,

@tonkolviktor
Copy link

tonkolviktor commented May 16, 2023

hello, thanks for looking into this issue!

Since the ticket about the cahce_* function was closed as a duplicate of this, could someone please change the title. At this point it's not only about an experimental feature, but about functions which are main stream (since cache() prints huge ugly warnings to the UI)

Supporting datetimes should be pretty basic functionality, I understand that you have many things to prioritize, but at least please do not remove cache until the cache_data is working properly.

I personally think the "workaround" is more harmful then staying with cache(). It requires to do a pretty big change, which is not a simple search&replace. At the same time a "final solution" to the problem must not contain the separation of datetime and tz, since it's exactly the point of the aware dts.

Final note, let me promote this answer here as well: https://discuss.streamlit.io/t/hide-cache-deprecation-warning/38800/6
This flag allows people to stay with cache() until cache_* has this bug fixed (either natively or via hash_funcs).

thanks and happy coding

@sfc-gh-jcarroll
Copy link
Collaborator

I believe this will be fixed with #6502 in the next release. Let us know if it doesn't seem to solve the issue.

@th0ger
Copy link

th0ger commented Jun 10, 2023

I believe this will be fixed with #6502 in the next release. Let us know if it doesn't seem to solve the issue.

That's not clear to me, could you elaborate with an example?

@sfc-gh-jcarroll
Copy link
Collaborator

The below should work fine in the next Streamlit release after the launch of hash_funcs argument for @st.cache_data (which has now replaced experimental_memo)

import pytz
from datetime import datetime

tz = pytz.timezone('Europe/Berlin')

@st.cache_data(hash_funcs={datetime: lambda x: x.strftime('%a %d %b %Y, %I:%M%p')})
def load_data(dt):
    return dt

now=datetime.now()
st.text(load_data(dt=now))

now_tz=tz.localize(datetime.now())
st.text(load_data(dt=now_tz))

@sfc-gh-jcarroll
Copy link
Collaborator

1.24.0 has been released:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature:cache Related to st.cache_data and st.cache_resource priority:P3 status:confirmed Bug has been confirmed by the Streamlit team type:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants