-
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
Import PyScript Version into Python Runtime #958
Conversation
for more information, see https://pre-commit.ci
version_dict["releaselevel"] = version_parts[3] | ||
except IndexError: | ||
version_dict["releaselevel"] = "" | ||
|
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.
ah, this seems to contradict my previous comment. But then I don't understand how "normal" versions are supposed to like, with or without the .final
?
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.
So I figured if the "canonical" name of the version is "2022.09.1.RC2", then version_info
should match that, as version_info(year=2022, month=9, minor=1, releaselevel='RC2')
But if the cannonical name of the version is "2022.09.1", then either
- version-info should respect that the releaselevel is nothing
version_info(year=2009, month=9, minor=1, releaselevel='')
(this is what the PR currently does)
Or
- since we've been using "2022.09.1" to mean the actual release of "2022.09.1", we could put a signifier in the releaselevel for this
version_info(year=2009, month=9, minor=1, releaselevel='final')
The term "final" I'm not set on - could be "release" or something.
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.
I am inclined towards final
or release
instead of ""
but have no strong opinions to be honest.
Sadly, this breaks in These lines build fine - if you open a REPL you can see the two new values: pyscript/pyscriptjs/src/pyodide.ts Lines 74 to 75 in 2740e98
But
I can't for the life of me figure out why the second Any ideas? |
Apologies for the delay Jeff! The pyodide unit tests are importing pyodide and loading it dynamically, I wonder if maybe that's why it's it's complainig that PyScript is not available? @madhur-tandon perhaps you have more context into this? 🤔 |
Thanks @FabioRosado ! For an extra bit of weirdness, if I change that bit of this.run(pyscript as string);
logger.info("Dir Results: " + this.run('dir()')) Then this shows up in the jest logs: console.info
[pyscript/pyodide] Dir Results: ['__annotations__', '__builtins__', '__doc__', '__loader__', '__name__', '__package__', '__spec__', 'pyversion', 'version_info'] So |
Ok some further oddities which make me think it's something odd about async interactions between pyodide and jest. Both of the following examples work when building for the browser, but... This (the above example) breaks jest: //main.ts
async afterRuntimeLoad(){
...
await runtime.loadInterpreter();
...
}
//pyodide.ts
async loadInterpreter(){
...
this.run(pyscript as string);
this.run(`PyScript.set_version_info('${version}')`)
...
} BUT this works in jest: //main.ts
async afterRuntimeLoad(){
...
await runtime.loadInterpreter();
runtime.run(`PyScript.set_version_info('${version}')`)
...
}
//pyodide.ts
async loadInterpreter(){
...
this.run(pyscript as string);
....
} 🤯 Not proposing this as a solution, it's just... odd and interesting. |
That's interesting, the pyodide tests are doing this:
I wonder if this is related? 🤔 Wonder if perhaps runtime didn't finish load (not sure if this even makes sense haha) |
I figured it out! jest is using a mocked version of And that mock is.... empty! So of course "PyScript isn't defined". I think the solution is: change |
Oh dang! That's a great find, awesome work! Copying over seems like a good idea or we could copy only the needed methods if that works, what do you think? |
Sorry for the late reply. Yeah, I did the mocking thing with an empty string because The project overall builds without failure since our pyscript/pyscriptjs/rollup.config.js Lines 44 to 46 in 0e1c396
aka the contents of Else, it seems like copying relevant stuff that is needed into the mocked string should be decently sufficient I guess. |
You can get jest to accept .py files with the config:
But then the tests explode because of the python imports
Maybe we go for the easier path and expand the mocked file 😄 |
Thanks y'all - for now, I've added pyscript/pyscriptjs/__mocks__/_pyscript.js Lines 13 to 14 in af24043
All other .py files are still caught by the original mapping and mocked with pyscript/pyscriptjs/__mocks__/fileMock.js Lines 13 to 16 in af24043
|
*/ | ||
|
||
const fs = require('fs'); | ||
module.exports = fs.readFileSync('./src/python/pyscript.py', 'utf8'); |
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.
Apologies a bit late to the party but... NICE! 😄
After Ted's PR earlier this week (#952), here's a small-ish PR to access that version in Python as well. It adds
PyScript.__version__
andPyScript.version_info
:This fixes #546 , and supercedes #833.