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

from_dict() method to fully set st.query_params, similar to experimental_set_query_params #8407

Closed
2 tasks done
sfc-gh-jcarroll opened this issue Apr 2, 2024 · 17 comments · Fixed by #8470
Closed
2 tasks done
Labels
feature:query-params type:enhancement Requests for feature enhancements or new features

Comments

@sfc-gh-jcarroll
Copy link
Collaborator

sfc-gh-jcarroll commented Apr 2, 2024

Checklist

  • I have searched the existing issues for similar feature requests.
  • I added a descriptive title and summary to this issue.

Summary

Provide a single method on st.query_params which takes a dict input and does the following as a single operation: removes any existing keys and sets the query params to match the input dict.

Why?

We have seen some advanced apps that manage multiple query params and want to set them simultaneously, and/or set some query_params and remove all others based on some user action.

In this case, the new st.query_params current functionality is actually worse than what was possible with st.experimental_set_query_params, as two updates are needed:

st.query_params.clear()
st.query_params.update(new_values)

It would be nice to do this as one operation, especially since it has consequences on the browser history.

How?

The API should have basically the same behavior as experimental_set_query_params, except taking a single dict argument instead of arbitrary kwargs that are converted to a dict. It should be a method on st.query_params instead of a standalone function.

After discussion we agreed on: st.query_params.from_dict()

Additional Context

See discussion in

@sfc-gh-jcarroll sfc-gh-jcarroll added the type:enhancement Requests for feature enhancements or new features label Apr 2, 2024
Copy link

github-actions bot commented Apr 2, 2024

To help Streamlit prioritize this feature, react with a 👍 (thumbs up emoji) to the initial post.

Your vote helps us identify which enhancements matter most to our users.

Visits

@sfc-gh-jcarroll
Copy link
Collaborator Author

@Asaurus1 @sfc-gh-bhay @sfc-gh-bhess @marcotielen WDYT? Thoughts or other ideas on the method name? I think set_dict() is my favorite of the ones I brainstormed (or cribbed)

cc @willhuang1997 @vdonato @LukasMasuch in case you have thoughts. Otherwise I'd be happy for someone to contribute this 😃

@Asaurus1
Copy link
Contributor

Asaurus1 commented Apr 2, 2024

I like set_all, as an aptly describes what you're doing which is setting all of the query params. It's just unfortunate that you named the operation to get a specific key as a list value "get_all" because I also agree that the fact it's not symmetrical is a bit grating 🫠. Perhaps "set_entire" or "set_every" could work.

I don't particularly like set_dict: if dict is meant to describe the thing you're passing in, I don't think it belongs in the name and we wouldn't want to restrict the input to a dictionary, it could be any mapping that supports keys and get item. Likewise, if it's meant to describe the thing that query params are, that's also a bit misleading because even though they're stored internally in a dictionary, they're not exactly the same thing. I think "set_params" would be preferable even though the word "params" would be doubled up on.

"Set" while short might conflict with existing keys so I think we should avoid it.

@Asaurus1
Copy link
Contributor

Asaurus1 commented Apr 2, 2024

You could also do "from_dict" which would at least mirror well with "to_dict"... "from_mapping" might be technically more correct but eh... I could live with it.

@Asaurus1
Copy link
Contributor

Asaurus1 commented Apr 2, 2024

Here's a draft PR for review, once we agree on a name can add things like tests + type proxy functions + all that fun stuff.
Asaurus1#4

@marcotielen
Copy link

I vote from_dict . Nicely in line with pandas. The only unfortunate part is that to_dict has a different behavior for lists (repeating keys). But who knows what the future can bring on that one.

@Asaurus1
Copy link
Contributor

Asaurus1 commented Apr 2, 2024

I vote from_dict . Nicely in line with pandas. The only unfortunate part is that to_dict has a different behavior for lists (repeating keys). But who knows what the future can bring on that one.

Yeah this is a good point. To_dict and from_dict won't be perfect mirrors due to certain design decisions that were made. I.e. params.from_dict(params.to_dict()) is not an identity operation.

@marcotielen
Copy link

I can live with that… the intent is there. But I’m the newbie here 🙂

@sfc-gh-bhay
Copy link
Contributor

@sfc-gh-jcarroll I think .set_all() is concise and descriptive enough. That being said I'm agnostic on the name you guys settle on.

@sfc-gh-jcarroll
Copy link
Collaborator Author

One other idea would be st.query_params.update(..., clear=True), although update() also accepts arbitrary kwargs as keys so it would slightly break / deviate that behavior 🤔 but otherwise I like that API a lot.

@Asaurus1
Copy link
Contributor

Asaurus1 commented Apr 4, 2024

One other idea would be st.query_params.update(..., clear=True), although update() also accepts arbitrary kwargs as keys so it would slightly break / deviate that behavior 🤔 but otherwise I like that API a lot.

Imo that would be way too easy to accidentally pass. You could call .update(foo=1, **extrasdict) and suddenly half of your query params disappear because this dict had a "clear" key in it.

@marcotielen
Copy link

marcotielen commented Apr 4, 2024

@Asaurus1 , @sfc-gh-bhay , @sfc-gh-jcarroll , @sfc-gh-wihuang , @willhuang1997 , @LukasMasuch : vote? not sure how to create a poll, but the below will do. Vote with an emoticon.

st.query_params.set_dict() > thumbs up
st.query_params.set_all() > smile
st.query_params.replace() > party
st.query_params.override() > heart/love
st.query_params.from_dict() > rocket
non of the above > thinking face

@Asaurus1
Copy link
Contributor

Asaurus1 commented Apr 5, 2024

Well all right well unless the other three are going to all vote for the same one we seem to have a clear choice here, I will be AFK for the next few days. Will or Joshua or marcotielien feel free to take the code from my branch above and run with it.

@marcotielen
Copy link

marcotielen commented Apr 8, 2024

@Asaurus1

I'm on windows and not a regular streamlit dev. So I just took the nightly build and adjusted below two files (streamlit\runtime\state).

query_params.txt
query_params_proxy.txt

test code (easy to verify through checking browser history):

import streamlit as st
import copy

my_dict = dict()
my_dict2 = dict()
my_dict3 = dict()

my_dict['page'] = 'hello'
my_dict['bla'] = 'aha'

if st.button('run 1st'):
    st.query_params.update(my_dict)

if st.button('run 2nd - current'):
    my_dict2 = copy.deepcopy(my_dict)
    del my_dict2['bla']
    st.query_params.clear()
    st.query_params.update(my_dict2)

if st.button('run 2nd - silent clear'):
    my_dict3 = copy.deepcopy(my_dict)
    del my_dict3['bla']
    st.query_params.from_dict(my_dict3)

@sfc-gh-jcarroll sfc-gh-jcarroll changed the title Method to fully set st.query_params, similar to experimental_set_query_params from_dict() method to fully set st.query_params, similar to experimental_set_query_params Apr 8, 2024
@sfc-gh-jcarroll
Copy link
Collaborator Author

Let's go with from_dict(), I updated the issue accordingly.

@Asaurus1
Copy link
Contributor

Asaurus1 commented Apr 9, 2024

Ready for review. @marcotielen I tested your code above with this version and I think it's working as expected.

@marcotielen
Copy link

@Asaurus1 I did notice that both .update and .from_dict don’t have telemetry. Is that needed?

vdonato added a commit that referenced this issue Apr 13, 2024
## Describe your changes
Implements an atomic way to overwrite all query_params

## GitHub Issue Link (if applicable)
Closes #8407

## Testing Plan
Unit tests for query_params and query_params_proxy.

---------

Co-authored-by: Vincent Donato <vincent@streamlit.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature:query-params type:enhancement Requests for feature enhancements or new features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants