Skip to content

Transform relative index url in to absolutes one - #150

Merged
ryanking13 merged 6 commits into
pyodide:mainfrom
Carreau:winfo
Oct 31, 2024
Merged

Transform relative index url in to absolutes one#150
ryanking13 merged 6 commits into
pyodide:mainfrom
Carreau:winfo

Conversation

@Carreau

@Carreau Carreau commented Oct 24, 2024

Copy link
Copy Markdown
Contributor

And use 'PYPI' as an alias for the Pypi index.

Basically some indexes (like pypi.anaconda.org), use relative urls for
wheels, thus we need to resolve those before calling wheel.download()
or it tries to dowload relative to current page.

With this: https://github.com/carreau/cors running on localhost 8787

Manually install micropip:

js> pyodide.loadPackage('https://files.pythonhosted.org/packages/08/aa/cc0199a5f0ad350994d660967a8efb233fe0416e4639146c089643407ce6/packaging-24.1-py3-none-any.whl'); pyodide.loadPackage('http://localhost:8088/micropip-0.6.2.dev3+g90f4991.d20241024-py3-none-any.whl')

I get a proper install of IPython nightly from an anaconda repo:

>>> import micropip
>>> await micropip.install('ipython', index_urls=['http://localhost:8787/scientific-python-nightly-wheels/simple', 'PYPI'], verbose=True)
Collecting ipython
  Downloading ipython-8.29.0.dev0-py3-none-any.whl
Collecting traitlets>=5.13.0
  Downloading traitlets-5.14.3-py3-none-any.whl
Collecting stack-data
  Downloading stack_data-0.6.3-py3-none-any.whl
Collecting matplotlib-inline
  Downloading matplotlib_inline-0.1.7-py3-none-any.whl
Collecting prompt-toolkit<3.1.0,>=3.0.41
  Downloading prompt_toolkit-3.0.48-py3-none-any.whl
Collecting decorator
  Downloading decorator-5.1.1-py3-none-any.whl
Requirement already satisfied: traitlets (5.14.3)
Collecting jedi>=0.16
  Downloading jedi-0.19.1-py2.py3-none-any.whl
Collecting pygments>=2.4.0
  Downloading pygments-2.18.0-py3-none-any.whl
Collecting asttokens>=2.1.0
  Downloading asttokens-2.4.1-py2.py3-none-any.whl
Collecting executing>=1.2.0
  Downloading executing-2.1.0-py2.py3-none-any.whl
Collecting pure-eval
  Downloading pure_eval-0.2.3-py3-none-any.whl
Collecting wcwidth
  Downloading wcwidth-0.2.13-py2.py3-none-any.whl
Collecting parso<0.9.0,>=0.8.3
  Downloading parso-0.8.4-py2.py3-none-any.whl
Collecting six>=1.12.0
  Downloading six-1.16.0-py2.py3-none-any.whl
Installing collected packages: traitlets, decorator, matplotlib-inline, pygments, executing, pure-eval, wcwidth, prompt-toolkit, parso, jedi, six, asttokens, stack-data, ipython
Successfully installed traitlets-5.14.3, decorator-5.1.1, matplotlib-inline-0.1.7, pygments-2.18.0, executing-2.1.0, pure-eval-0.2.3, wcwidth-0.2.13, prompt-toolkit-3.0.48, parso-0.8.4, jedi-0.19.1, six-1.16.0, asttoke
ns-2.4.1, stack-data-0.6.3, ipython-8.29.0.dev0

(note the dev0 for IPython).
Well IPython does not import because no SQLITE3... but it's a progress.

--

On top of #145

@Carreau Carreau changed the title Add implied assertion in WheelInfo Transform relative index url in to absolutes one Oct 24, 2024
@Carreau
Carreau force-pushed the winfo branch 2 times, most recently from 9450394 to 223649b Compare October 24, 2024 13:28
@Carreau
Carreau marked this pull request as ready for review October 24, 2024 13:41
@hoodmane

Copy link
Copy Markdown
Member

I merged #145, could you rebase onto main to reduce the diff?

And use 'PYPI' as an alias for the Pypi index.

Basically some indexes (like pypi.anaconda.org), use relative urls for
wheels, thus we need to resolve those before calling `wheel.download()`
or it tries to dowload relative to current page.

With this: https://github.com/carreau/cors  running on localhost 8787

Manually install micropip:

js> pyodide.loadPackage('https://files.pythonhosted.org/packages/08/aa/cc0199a5f0ad350994d660967a8efb233fe0416e4639146c089643407ce6/packaging-24.1-py3-none-any.whl'); pyodide.loadPackage('http://localhost:8088/micropip-0.6.2.dev3+g90f4991.d20241024-py3-none-any.whl')

And

>>> import logging
... log = logging.getLogger('micropip')
... log.setLevel(10)
... import micropip
... await micropip.install('ipython', index_urls=['http://localhost:8787/scientific-python-nightly-wheels/simple', 'PYPI'])

I get a proper install of IPython nightly from an anaconda repo:

>>> import micropip
>>> await micropip.install('ipython', index_urls=['http://localhost:8787/scientific-python-nightly-wheels/simple', 'PYPI'], verbose=True)
Collecting ipython
  Downloading ipython-8.29.0.dev0-py3-none-any.whl
Collecting traitlets>=5.13.0
  Downloading traitlets-5.14.3-py3-none-any.whl
Collecting stack-data
  Downloading stack_data-0.6.3-py3-none-any.whl
Collecting matplotlib-inline
  Downloading matplotlib_inline-0.1.7-py3-none-any.whl
Collecting prompt-toolkit<3.1.0,>=3.0.41
  Downloading prompt_toolkit-3.0.48-py3-none-any.whl
Collecting decorator
  Downloading decorator-5.1.1-py3-none-any.whl
Requirement already satisfied: traitlets (5.14.3)
Collecting jedi>=0.16
  Downloading jedi-0.19.1-py2.py3-none-any.whl
Collecting pygments>=2.4.0
  Downloading pygments-2.18.0-py3-none-any.whl
Collecting asttokens>=2.1.0
  Downloading asttokens-2.4.1-py2.py3-none-any.whl
Collecting executing>=1.2.0
  Downloading executing-2.1.0-py2.py3-none-any.whl
Collecting pure-eval
  Downloading pure_eval-0.2.3-py3-none-any.whl
Collecting wcwidth
  Downloading wcwidth-0.2.13-py2.py3-none-any.whl
Collecting parso<0.9.0,>=0.8.3
  Downloading parso-0.8.4-py2.py3-none-any.whl
Collecting six>=1.12.0
  Downloading six-1.16.0-py2.py3-none-any.whl
Installing collected packages: traitlets, decorator, matplotlib-inline, pygments, executing, pure-eval, wcwidth, prompt-toolkit, parso, jedi, six, asttokens, stack-data, ipython
Successfully installed traitlets-5.14.3, decorator-5.1.1, matplotlib-inline-0.1.7, pygments-2.18.0, executing-2.1.0, pure-eval-0.2.3, wcwidth-0.2.13, prompt-toolkit-3.0.48, parso-0.8.4, jedi-0.19.1, six-1.16.0, asttoke
ns-2.4.1, stack-data-0.6.3, ipython-8.29.0.dev0
>>>

(Well IPyhton does not import because no SQLITE3... but it's a progress.
@Carreau

Carreau commented Oct 25, 2024

Copy link
Copy Markdown
Contributor Author

rebased.

Comment thread micropip/package_index.py Outdated
def from_json_api(data: str | bytes | dict[str, Any]) -> "ProjectInfo":
def from_json_api(
data: str | bytes | dict[str, Any], index_base_url: str
) -> "ProjectInfo":

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even if not used (yet?) I added the parameter to all the from_... method to make the signature consistant.

Comment thread micropip/package_index.py
version=version,
sha256=sha256,
size=size,
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just avoids a closure by making name a parameter, and making things a bit more explicit.

@hoodmane

Copy link
Copy Markdown
Member

Seems like this needs some new test cases to cover the new logic.

@ryanking13 ryanking13 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Generally looks okay to me. But I think it needs tests and changelog.

Comment thread micropip/package_index.py Outdated

@staticmethod
def from_json_api(data: str | bytes | dict[str, Any]) -> "ProjectInfo":
def from_json_api(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about setting the default value, so we don't need to pass index_base_url=""?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then better remove it; I just don't know of any json index that use relative URL; but I though it could happen.

Comment thread micropip/package_index.py

@classmethod
def _compatible_wheels(
cls, files: list[dict[str, Any]], version: Version, name: str

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some reason to make this a classmethod?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's easier to reason about than having a closure, where you are not sure what parameter it closes over (it only closed over name).

I can make it static; but can't make it an instance method as it's called from a class method already.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make it static; but can't make it an instance method as it's called from a class method already.

Oh you are right. I forgot that _compatible_only is already a classmethod...

Comment thread micropip/package_index.py Outdated
elif isinstance(index_urls, str):
index_urls = [index_urls]

index_urls = [PYPI_URL if url == "PYPI" else url for url in index_urls]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's define PYPI as a variable.

@Carreau

Carreau commented Oct 30, 2024

Copy link
Copy Markdown
Contributor Author

The black_simple_html.gz has been modified to remove
https://files.pythonhosted.org/ from some of the wheels url.

    <a href="https://files.pythonhosted.org/packages/...." </a><br />

to:

    <a href="/packages/...." </a><br />

Note that with the recent xz debacle, maybe we should not have binary
blobs in the test suite, it also seem to be that git is compressing
things anyway, so it might be counter productive ?

unless there is another reason to gz them.

The black_simple_html.gz has been modified to remove
https://files.pythonhosted.org/ from some of the wheels url.

    ```
    <a href="https://files.pythonhosted.org/packages/...." </a><br />
    ```
    to:
    ```
    <a href="/packages/...." </a><br />
    ```

Note that with teh recent xz debacle, maybe we should not have binary
blobs in the test suite, it also seem to be that git is  compressing
things anyway, so it might be counter productive ?
@ryanking13

Copy link
Copy Markdown
Member

unless there is another reason to gz them.

I gz-ed them so they don't show in the grep or in the search result, but I think your point makes sense and I am okay with using plain text files. Feel free to decompress them in this PR.

@ryanking13 ryanking13 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Please update the changelog.

@Carreau

Carreau commented Oct 31, 2024

Copy link
Copy Markdown
Contributor Author

I gz-ed them so they don't show in the grep or in the search result, but I think your point makes sense and I am okay with using plain text files. Feel free to decompress them in this PR.

I think I'm going to do a separate one for readability.

@ryanking13

Copy link
Copy Markdown
Member

I think I'm going to do a separate one for readability.

Sure. Thanks!

@ryanking13
ryanking13 merged commit d4ba560 into pyodide:main Oct 31, 2024
@Carreau

Carreau commented Oct 31, 2024

Copy link
Copy Markdown
Contributor Author

Thanks.

Carreau added a commit to Carreau/micropip that referenced this pull request Oct 31, 2024
ryanking13 pushed a commit that referenced this pull request Oct 31, 2024
* Do not store pypi_response as gz.

This uncompress all files in `pypi_response` and update the relevant
section of test code to deal with the new format.

The idea is that binary blobs as test data are frowned upon for various
reasons, even more recenlty with the `xz` fiasco where there were used
to carry a backdoor.

Compression does not really reduce repository size as git is compressed
anyway;

And uncompressed files are harder to modify/review modifications.

* show explit changes of #150

* Same changes as previously but WRT metadata files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants