Skip to content

Change to support patching OpenerDirector objects in urllib#20

Merged
koenvo merged 1 commit intokoenvo:mainfrom
xangma:urllib_od
Nov 16, 2022
Merged

Change to support patching OpenerDirector objects in urllib#20
koenvo merged 1 commit intokoenvo:mainfrom
xangma:urllib_od

Conversation

@xangma
Copy link
Contributor

@xangma xangma commented Nov 16, 2022

This change patches over the open method for any created urllib OpenerDirector objects with the urlopen method here. OpenerDirectors are used in astropy here.

This change patches over the open method for any created urllib OpenerDirector objects with the urlopen method here. OpenerDirectors are used in astropy [here](https://github.com/astropy/astropy/blob/main/astropy/utils/data.py#L1211).
@koenvo koenvo merged commit e5ee4a5 into koenvo:main Nov 16, 2022
@koenvo
Copy link
Owner

koenvo commented Nov 16, 2022

Thanks for this PR. As far as I can see this doesn't help astropy as it still tries to import ssl (https://github.com/astropy/astropy/blob/main/astropy/utils/data.py#L1199). This still fails.

@xangma
Copy link
Contributor Author

xangma commented Nov 16, 2022

Ah, I understand why my code works now.

The reason I patched OpenerDirector was to use pyscript to run the pyvo package - an astropy affiliated package. It makes an astropy function call to download a file when running the pyvo.registry.search function. When I set up pyscript with the py-config, I also import ssl, which works for some reason I can't remember unfortunately, and that allows that astropy urlopen function to work. Does this make sense?

Here is my test code:

<py-config>
    packages = ["pyvo", "ssl", "pyodide-http"] 
</py-config>

<py-script>
    import pyodide_http
    pyodide_http.patch_all()
    from pyvo import registry
    res = registry.search(servicetype = 'conesearch', waveband = 'UV')
    print(res)
</py-script>

I should say, it says pyodide-http in the py-config above, which will do a micropip install I think? In my local code it pointed to a locally compiled whl instead with this PR in it.

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.

2 participants