-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add tests for runtime config inside py-config and remove usage of indexURL #734
Conversation
9d0089d
to
a481a2d
Compare
TLDR: Lessons learnt: Avoid using Issue: Solution:
I have added comments to explain the JS voodo I did. Consequence: All tests will pass and manually using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @madhur-tandon: I suppose it was not an easy task to debug the indexURL
issue, good job!
In general I'm very happy with this PR; I suggested a number of changes to make it even better. Don't be scared by the number of comments, most are shallow :).
I think this PR is missing a test, though. As I wrote in the comments, the current test_config
is misnamed because it should be test_runtimes
. But indeed, we do miss a test_config
, i.e. the simplest possible test to check that <py-config>
is loaded and evaluated by pyscript. Ideally, it should be placed before test_runtimes
, because it tests a basic functionality which is a prerequisite for test_runtimes
to work.
Looking at AppConfig
, it seems we can also set the name
and version
of our app:
pyscript/pyscriptjs/src/runtime.ts
Lines 23 to 28 in 7be72ee
export type AppConfig = { | |
autoclose_loader: boolean; | |
name?: string; | |
version?: string; | |
runtimes?: Array<RuntimeConfig>; | |
}; |
So, a good way to write a test_config
could be:
- add some logging inside our typescript files to
console.log
appConfig_.name
and.version
if they are set (which looks generally useful) - write a minimal test which sets them via
<py-config>
- check that the correct name and version are printed to
console.log
Did you check the file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, left minor comments and, as @antocuni mentioned, can be addressed in a follow up PR
TMP_TAR_LOCATION = os.path.join(TMP_DIR, TAR_NAME) | ||
with open(TMP_TAR_LOCATION, "wb") as f: | ||
f.write(response.raw.read()) | ||
val = TMP_TAR_LOCATION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
veeeery nitpicking, but note that here you are loading 151MB in memory all at once, just to write them to disk. I'm sure there are better ways to save an http request to disk.
That said, it's not a blocker for this PR, but just wanted to say it as a general comment/suggestion for the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chunking perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chunking is surely a good option.
I would have expected requests
to have a direct way to save to disk but apparently there isn't.
A quick google reveals this.
@hoodmane You might be able to chime in here.
The test loads the
pyodide.js
file which is present undernode_modules/pyodide
.This further leads to loading of
pyodide_py.tar
andpyodide.asm.js
While
/pyodide/pyodide.js
and/pyodide/pyodide_py.tar
are loaded correctly, the path for the last one is always wrong.It is
/build/pyodide/pyodide.asm.js
instead of just/pyodide/pyodide.asm.js
What could be the reason for this? I also tried downloading the release from here: https://github.com/pyodide/pyodide/releases/tag/0.21.1
and using the downloaded folder as the path to
indexURL
(instead of the one from undernode_modules/pyodide
).It seems like somewhere the path for where to look at
pyodide.asm.js
is set incorrectly. Can you confirm this?Maybe it is related to the issue fixed here: pyodide/pyodide#3015?