search

brandon-rhodes / python-jplephem

Python version of NASA DE4xx ephemerides, the basis for the Astronomical Alamanac
MIT License
111 stars 29 forks source link

pyodide: no mmap #39

Closed jcaesar closed 4 years ago

jcaesar commented 4 years ago

I tried running skyfield on pyodide. The only real problem I ran into was that mmap failed after acquiring a fileno. I figured that the patch is innocent enough that you might take it.

brandon-rhodes commented 4 years ago

Thanks for letting me know about this! One tweak: I generally target exception handlers to only the single line that could possibly raise the exception. In this diff, I can't tell whether lines like skip = i % mmap.ALLOCATIONGRANULARITY and r = mmap.ACCESS_READ are in danger of raising an exception, or whether it's only m = mmap.mmap(fileno, length=j-i+skip, access=r, offset=i-skip) that can fail?

Is there any way to know ahead of time whether mmap() is available, or is trying it the only way? I've not used a platform yet that had this limitation.

What does the comment canary mean? It might not be clear to future readers of the code.

But aside from those small items, this looks good at first read and I'll be happy to accept this contribution once it's trimmed up. Thanks!

jcaesar commented 4 years ago

Wow, swift response.

The actual error from mmap looks like this

  File "/lib/python3.7/site-packages/jplephem/daf.py", line 114, in map_words
    m = mmap.mmap(fileno, length=j-i+skip, access=r, offset=i-skip)
OSError: [Errno 19] No such device

I don't know whether it is possible to predict that error.

I've updated the patch so that the exception handlers only target one line (and removed the canary. Sorry, that wasn't supposed to be there). I think it looks a little bit contrived. If you'd like me to rewrite it with a sub-def or any other way, please do tell.

brandon-rhodes commented 4 years ago

Thanks for the update! I think the logic is much clearer now. The additional level of indentation is unusual but expresses very nicely the two stages of possible failure the process of opening and mmap'ing the file might deliver.

I see one more tiny tweak, but I'll make it myself later today when I have time to sit down and do the merge and write a test.

Then I'll see about getting a release out.

brandon-rhodes commented 4 years ago

Wow, swift response.

Credit goes to the browser plugin "Notifier for GitHub" :)

https://chrome.google.com/webstore/detail/notifier-for-github/lmjdlojahmbbcodnpecnjnmlddbkjhnn

brandon-rhodes commented 4 years ago

There! I've just released version 2.14 with this new feature. Let me know if you have any further problems, and thanks again for the report!

jcaesar commented 4 years ago

Awesome. (Also: try...except...else. I learned something new. Thanks again.)