Showing an error when it's not localhost and has no mapbox token by arraydude · Pull Request #1295 · streamlit/streamlit

arraydude · 2020-04-02T23:34:00Z

Issue: Fix #1136 #812

Description: As it is not possible to render a Mapbox without any token we are delimiting the use locally.

Prevent fetching token when it's not localhost or hello.py
Using axios instead of fetch
Added new step for cypress job in which we set the mapbox token
Unittest

Contribution License Agreement

By submiting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

frontend/src/hocs/withMapboxToken/MapboxToken.ts

… showingTokenErrorWhenNotLocalhost

… runs, so I moved it to init config

tvst · 2020-04-30T05:58:04Z

frontend/src/hocs/withMapboxToken/MapboxToken.ts

+            <a href="https://mapbox.com">https://mapbox.com</a>. It's free! (for moderate usage levels) See
+            <a href="https://docs.streamlit.io/cli.html#view-all-config-options">our documentation</a> for more
+            info on how to set config options.
+            `


Hey, I was going to comment on this error message, but I need to see it in action to really understand what it should say. Can you give me an example of how to see it?

you could run for example /e2e/scripts/st_map.py without setting any mapbox token within your config.toml

This is how it looks

I see the following snippet when I run streamlit config view:

[mapbox] # Configure Streamlit to use a custom Mapbox token for elements like st.deck_gl_chart and st.map. If you don't do this you'll be using Streamlit's own token, which has limitations and is not guaranteed to always work. To get a token for yourself, create an account at https://mapbox.com. It's free! (for moderate usage levels) # Default: "" token = ""

I wonder if we need to change the comment in the config, because with these change the user will no longer be using Streamlit's own token unless they are running streamlit hello locally.

EDIT: I believe the config comments are at

streamlit/lib/streamlit/config.py

Line 538 in 908827b

_create_option(

Good catch!, thank you @Amey-D

tvst · 2020-04-30T06:11:44Z

I just realized, before we release this change we should sync with @randyzwitch on how to inform the community, and what changes we need to add for our docs.

(Randy, if you're reading this and are confused about what's going on, PTAL at #1136 for more info)

Amey-D

Overall looks great! Added a couple of suggestion primarily to minimize user confusion around this change. PTAL.

frontend/src/hocs/withMapboxToken/MapboxToken.ts

Amey-D · 2020-04-30T18:23:02Z

frontend/src/hocs/withMapboxToken/MapboxToken.ts

+            <a href="https://mapbox.com">https://mapbox.com</a>. It's free! (for moderate usage levels) See
+            <a href="https://docs.streamlit.io/cli.html#view-all-config-options">our documentation</a> for more
+            info on how to set config options.
+            `


I see the following snippet when I run streamlit config view:

[mapbox] # Configure Streamlit to use a custom Mapbox token for elements like st.deck_gl_chart and st.map. If you don't do this you'll be using Streamlit's own token, which has limitations and is not guaranteed to always work. To get a token for yourself, create an account at https://mapbox.com. It's free! (for moderate usage levels) # Default: "" token = ""

I wonder if we need to change the comment in the config, because with these change the user will no longer be using Streamlit's own token unless they are running streamlit hello locally.

EDIT: I believe the config comments are at

streamlit/lib/streamlit/config.py

Line 538 in 908827b

_create_option(

Co-authored-by: Amey Deshpande <[email protected]>

…t' into showingTokenErrorWhenNotLocalhost

Co-authored-by: Amey Deshpande <[email protected]>

randyzwitch · 2020-04-30T20:12:47Z

I just realized, before we release this change we should sync with @randyzwitch on how to inform the community, and what changes we need to add for our docs.

Got it, ran into this same problem at my previous company, so familiar with the general idea.

I think we could make a forum post right away in Announcements to highlight our intentions; regardless of what implementation or deprecation decisions are being made, we've already decided that users in production must use their own key.

Once we think through the actual policy, then we can both update the Announcement forum page, as well as call it out in the changelog. I don't think we should necessarily worry about breaking old code after a 60-90 grace period; so throw a deprecation warning starting with the next release with a firm date in the message (July 31, 2020?), and then turn it off on the date we say in the deprecation warning.

randyzwitch · 2020-04-30T20:17:31Z

If we want to be even more conservative, we could publish a blog post 30 days before we turn it off, to try and reach as many people as possible.

tvst

Let's wait for at least one release before merging. Marking as "request changes" to block this in the meantime.

tvst · 2020-05-01T21:16:53Z

Given that this PR (and this issue) went through a thousand different specs/rewrites, I'd love to get it in "as is" rather than do more work on it.

With that in mind, how about we do the release like this:
(IIUC this is basically @randyzwitch's plan but with some dates tweaked)

First do a forum post
Wait N_MERGE weeks. Do normal releases throughout this time.
Merge this code.
Do a release. The release would behave this way:
- Users on old Streamlit versions would not see any changes (they'd be using our old token, "token0")
- Users on new Streamlit versions would not be able to use DeckGL maps without a token (the exception is hello.py, which would work by using "token1". This token only works on localhost, by the way)
Wait N_FINAL_ANNOUNCE weeks.
Make another announcement, this time saying old apps will stop working in N_DELETE weeks.
Wait N_DELETE weeks.
Delete token0.

From the eng side, it's best if N_MERGE is 2 weeks. As for N_FINAL_ANNOUNCE and N_DELETE, those don't matter to eng.

@randyzwitch would you be comfortable with this?

frontend/src/hocs/withMapboxToken/withMapboxToken.tsx

tvst · 2020-05-02T06:14:24Z

I found another problem.

Try this:
0) Make sure you don't have a Mapbox token configured

streamlit hello
Go to the map example
Wait for the map to show up
Go on your terminal (but don't close the browser tab!) and ctrl-c from the Streamlit server
Now run streamlit run e2e/scripts/st_pydeck_chart.py

Expected: the charts should not show up. You should see 2 error messages instead.
Actual: you see two charts!

What's happening: the Mapbox token that was fetched with streamlit hello is hanging around in the tab, even if you change apps.

… showingTokenErrorWhenNotLocalhost

tvst · 2020-05-13T06:29:58Z

The backticks in the error are printing as actual backticks, not as inline <code>:

Also, when users have an st.map, the error mentions st.pydeck_chart rather than st.map. I think we can solve this by making the error mention both instead, like this:

To use st.pydeck_chart or st.map you need (...)

tvst

Left some comments, check below

frontend/src/components/elements/DeckGlChart/DeckGlChart.tsx

frontend/src/hocs/withMapboxToken/MapboxTokenError.tsx

frontend/src/hocs/withMapboxToken/MapboxToken.ts

tvst · 2020-05-13T06:42:45Z

.circleci/config.yml

            mkdir ~/.streamlit
+            MAPBOX_TOKEN=$(curl -sS https://data.streamlit.io/tokens.json | jq -r '.mapbox')
+            echo '[mapbox]' >  ~/.streamlit/config.toml
+            echo 'token = "'$MAPBOX_TOKEN'"' >> ~/.streamlit/config.toml


We need a solution for local e2e tests too

I added the same for the dockerfile

.circleci/config.yml

… showingTokenErrorWhenNotLocalhost

* develop: Enable WebSocket compression (#1506) Showing an error when it's not localhost and has no mapbox token (#1295) Reapply on_config_parsed after applying cli args (#1448)

Showing a MapBoxToken error when it's not localhost and has no token

2b4e585

arraydude requested a review from a team as a code owner April 2, 2020 23:34

Merge branch 'develop' into showingTokenErrorWhenNotLocalhost

dd9eb62

tconkling reviewed Apr 3, 2020

View reviewed changes

frontend/src/hocs/withMapboxToken/MapboxToken.ts Outdated Show resolved Hide resolved

arraydude added 2 commits April 10, 2020 15:03

Merge branch 'develop' into showingTokenErrorWhenNotLocalhost

9c26d6c

Merge branch 'develop' into showingTokenErrorWhenNotLocalhost

5fcc214

arraydude added the paused label Apr 14, 2020

Merge branch 'develop' of https://github.com/streamlit/streamlit into…

0b461c1

… showingTokenErrorWhenNotLocalhost

arraydude added WIP and removed paused labels Apr 28, 2020

Updating tests

93a16c4

arraydude removed the WIP label Apr 29, 2020

arraydude added 7 commits April 29, 2020 16:33

Merge branch 'develop' into showingTokenErrorWhenNotLocalhost

23cd4fb

Adding mapbox token to e2e tests

d69a9d0

Adding mapbox token to e2e tests

9a603ee

Fixing init config and credentials

7549c84

Merge branch 'develop' into showingTokenErrorWhenNotLocalhost

e2df052

Fixing init config and credentials

8bc570c

Removing get mapbox token step as it is not possible to share between…

9d51fbe

… runs, so I moved it to init config

arraydude requested a review from Amey-D April 29, 2020 22:14

tvst reviewed Apr 30, 2020

View reviewed changes

arraydude added 3 commits April 30, 2020 13:57

Using new localhost token

4d2c097

Updating tests with new token

669ec54

Merge branch 'develop' into showingTokenErrorWhenNotLocalhost

74bc412

Amey-D reviewed Apr 30, 2020

View reviewed changes

arraydude and others added 4 commits April 30, 2020 16:04

Update frontend/src/hocs/withMapboxToken/MapboxToken.ts

02a4d30

Co-authored-by: Amey Deshpande <[email protected]>

Merge branch 'develop' into showingTokenErrorWhenNotLocalhost

d9a8eb9

Updating mapbox.token config description

818a6a4

Merge remote-tracking branch 'origin/showingTokenErrorWhenNotLocalhos…

ec14b50

…t' into showingTokenErrorWhenNotLocalhost

Update lib/streamlit/config.py

fcbac4b

Co-authored-by: Amey Deshpande <[email protected]>

tvst suggested changes May 1, 2020

View reviewed changes

tvst suggested changes May 2, 2020

View reviewed changes

frontend/src/hocs/withMapboxToken/withMapboxToken.tsx Outdated Show resolved Hide resolved

frontend/src/hocs/withMapboxToken/withMapboxToken.tsx Show resolved Hide resolved

tvst mentioned this pull request May 2, 2020

Clean up use of LoDash #1404

Merged

arraydude added 6 commits May 8, 2020 17:43

Merge branch 'develop' of https://github.com/streamlit/streamlit into…

d9683db

… showingTokenErrorWhenNotLocalhost

Merge branch 'develop' of https://github.com/streamlit/streamlit into…

972d0aa

… showingTokenErrorWhenNotLocalhost

Mapbox token is refreshed when command line change

1ef3688

Fixing unit test

6c2cea7

Fixing unit test

f21efcc

Resolving @tvst comments

86c7d63

arraydude requested a review from tvst May 11, 2020 22:43

Printing delta type

48bf882

tvst suggested changes May 13, 2020

View reviewed changes

arraydude added 8 commits May 13, 2020 13:59

Errors wordings

fedbd31

Moving isHello to SessionInfo

6716960

DeltaType as a param

e2976d9

mapbox-localhost instead of mapbox

c3da970

Adding mapbox_token for local e2e

94638b4

Fixing cypress

00a9c4c

without compose function

79eb1f6

Merge branch 'develop' of https://github.com/streamlit/streamlit into…

9dc51c1

… showingTokenErrorWhenNotLocalhost

tvst approved these changes May 22, 2020

View reviewed changes

arraydude merged commit 0b633d2 into streamlit:develop May 27, 2020

arraydude deleted the showingTokenErrorWhenNotLocalhost branch May 27, 2020 22:20

Conversation

arraydude commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvst commented Apr 30, 2020

Uh oh!

Amey-D left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

randyzwitch commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

randyzwitch commented Apr 30, 2020

Uh oh!

tvst left a comment

Choose a reason for hiding this comment

Uh oh!

tvst commented May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tvst commented May 2, 2020

Uh oh!

tvst commented May 13, 2020

Uh oh!

tvst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arraydude commented Apr 2, 2020 •

edited

Loading

randyzwitch commented Apr 30, 2020 •

edited

Loading

tvst commented May 1, 2020 •

edited

Loading