ENH Add extra_index_urls and index_strategy parameters to micropip.install(<...>)#224
ENH Add extra_index_urls and index_strategy parameters to micropip.install(<...>)#224agriyakhetarpal wants to merge 5 commits intopyodide:mainfrom
extra_index_urls and index_strategy parameters to micropip.install(<...>)#224Conversation
|
This PR has also been ready for review, and I can work on it further so that we can include it in an upcoming 0.10.0 release. I'll re-trigger the tests now. If I remember correctly, the test failures I had encountered were simple to fix – I would love feedback on the API design. Please note that the behaviour deviates from Therefore, I think it makes sense for us to be security-aware as well, given the inherent security model for running WASM in browsers and the fact that WASM as a format in general has been proven not to be particularly effective against, say, cryptocurrency mining malware. |
agriyakhetarpal
changed the title
extra_index_urls and index_strategy parameters (DRAFT]extra_index_urls and index_strategy parameters
agriyakhetarpal
changed the title
extra_index_urls and index_strategy parametersextra_index_urls and index_strategy parameters to micropip.install(<...>)
There was a problem hiding this comment.
Thanks for working on this Agriya!
I am not sure about the extra_index_urls parameter, but having index_strategy sounds good to me. So how about separating the PR into two?
Also, I am not sure the current implementation is regarding the index strategy is correct, I left a comment about it,
| name: str, | ||
| index_urls: list[str] | str, | ||
| fetch_kwargs: dict[str, Any] | None = None, | ||
| strategy: str = "first-index", |
There was a problem hiding this comment.
For betting typing, how about using Literal type?
| - If a list of URLs is provided, micropip will try each URL in order until \ | ||
| it finds a package. If no package is found, an error will be raised. | ||
|
|
||
| extra_index_urls: |
There was a problem hiding this comment.
I am not sure if we need to separate extra_index_urls as we already support multiple index_urls.
How about exposing an API to get the current index URLs for micropip instead?
For example,
micropip.install("...", extra_index_urls=["A", "B", "C"])can be replaced with
current_index_urls = micropip.get_index_urls()
micropip.install("...", index_urls=current_index_urls + ["A", "B", "C"])|
|
||
| # With "first-index" strategy, we'll return the first match we find | ||
| # without checking the other indexes at all. | ||
| if strategy == "first-index": |
There was a problem hiding this comment.
Each strategy has duplicate logics, so I think we can separate this with multiple functions, extracting out common parts.
| raise ValueError(f"Error trying to decode url: {url}") from e | ||
| return parser(metadata) | ||
| # With "unsafe-first-match" or "unsafe-best-match", we need to check all indexes | ||
| else: |
There was a problem hiding this comment.
I think it is still better to check the strategy string, as people might pass some strange value here.
Or, we can probably check the value early and failfast.
| if strategy == "unsafe-first-match": | ||
| return projects_info[0] |
There was a problem hiding this comment.
Is it the right implementation? If the projects_info[0] does not have the compatible version, I think we should fallback to other indices.
The compatibility check is not handled in this function (at least in the current implementation). So I think so support this strategy, we'll need to modify the Transaction as well.
2 participants
Closes #223