Make Carto the default map provider (like in PyDeck)#11231
Merged
sfc-gh-tteixeira merged 27 commits intodevelopfrom May 8, 2025
Merged
Make Carto the default map provider (like in PyDeck)#11231sfc-gh-tteixeira merged 27 commits intodevelopfrom
sfc-gh-tteixeira merged 27 commits intodevelopfrom
Conversation
Contributor
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
Contributor
✅ PR preview is ready!
|
Contributor
📈 Frontend coverage change detectedThe frontend unit test (vitest) coverage has increased by 0.0300%
🎉 Great job on improving test coverage! |
kmcgrady
added
security-assessment-completed
change:feature
lukasmasuch
reviewed
May 6, 2025
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR changes Streamlit’s default map provider from Mapbox to Carto, eliminating the need for a default Mapbox API key. Key changes include deprecating the "mapbox.token" config option with migration guidance, removing the withMapboxToken HOC and its related tests/components, and updating the DeckGl configuration to use Carto’s basemaps and a built‐in Carto key when necessary.
Reviewed Changes
Copilot reviewed 66 out of 66 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/streamlit/config_util.py | Minor refactoring for comment formatting in appending deprecated option messages |
| lib/streamlit/config.py | Updated deprecation details for mapbox.token and revised docstrings for clarity |
| frontend/lib/src/components/elements/DeckGlJsonChart/useDeckGl.tsx | Updated default map style and added Carto key logic for Carto maps |
| frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.tsx | Updated map token logic and removed withMapboxToken usage |
| Several frontend and e2e test files | Removed references and tests related to Mapbox token fetching, adjusting test expectations accordingly |
kmcgrady
approved these changes
May 6, 2025
Collaborator
kmcgrady
left a comment
kmcgrady
left a comment
There was a problem hiding this comment.
Overall the code seems good. Lukas has a valid comment about restricting the e2e tests to one test if possible, but I expect that to be straightforward.
Note to @mayagbarnes and @sfc-gh-bnisco that I expect an update to SiS for the 1.46 release to occur for the CSP (see PR description). Just an FYI.
| append_comment( | ||
| "This option will be removed on or after %s." | ||
| % option.expiration_date | ||
| ) |
Collaborator
There was a problem hiding this comment.
nit: Can update this to a f-string.
sfc-gh-kmcgrady
approved these changes
May 6, 2025
Contributor
sfc-gh-kmcgrady
left a comment
sfc-gh-kmcgrady
left a comment
There was a problem hiding this comment.
Protobuf is backwards compatible
Contributor
📈 Python coverage change detectedThe Python unit test coverage has increased by 0.0263%
✅ Coverage change is within normal range. Coverage by files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Background:
st.pydeck_chart and st.map currently use Mapbox as their default map provider. However, Mapbox requires users to pass an API key first. In the past, we provided a default API key for users, but the whole thing always had a funny smell.
Somewhat recently, Pydeck changed their default map provider from Mapbox to Carto -- which does not require an API key. But this didn't apply to Streamlit users because we had overridden the default with Mapbox.
This PR addresses this.
What this PR does:
mapbox.tokenconfig option. We now refer users to Pydeck'sapi_keysarg orMAPBOX_API_KEYenv var. The old config option is still supported, though.streamlit config show.NOTE: For this to work in locked-down hosts (like Snowflake) they'll have to add this to their CSP allow list:
Or, if you want to be really picky...
...but I worry Carto may add mote tile servers in the future and this will be a forever game.
GitHub Issue Link (if applicable)
Testing Plan
Explanation of why no additional tests are neededAny manual testing needed?Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.