[vscode installation] 1/n Python version validation #1262
Conversation
Ankush-lastmile
force-pushed
the
pr1262
branch
from
ea09b0d
to
b1ed371
Compare
Ankush-lastmile
force-pushed
the
pr1262
branch
from
b1ed371
to
0088eaa
Compare
Ankush-lastmile
changed the title
Ankush-lastmile
force-pushed
the
pr1262
branch
2 times, most recently
from
c798acc
to
efd4e79
Compare
Ankush-lastmile
requested review from
saqadri,
rholinshead,
suyoglastmileai,
jonathanlastmileai and
rossdanlm
as code owners
| * @returns A promise that resolves to a boolean indicating if the version is >= 3.10. | ||
| */ | ||
|
|
||
| export function isPythonVersionAtLeast310(pythonPath: string): boolean { |
There was a problem hiding this comment.
I think this is ok to unblock but ideally if there's some way we can determine the required python version dynamically via pip or something that would be best
There was a problem hiding this comment.
what do you mean by dynamically?
also just noting that pip itself has versions, which is not the same as python's version
There was a problem hiding this comment.
Oh, what I mean is our pyproject.toml defines requires-python = ">=3.10" and that shows up as metadata in pip. Ideally we could dynamically get that version in the code without having to hardcode it
| export function isPythonVersionAtLeast310(pythonPath: string): boolean { | ||
| try { | ||
| // Use Python to check compatible version | ||
| const command = `${pythonPath} -c "import sys; print(sys.version_info >= (3, 10))"`; |
There was a problem hiding this comment.
Why this command vs just doing --version check? IMO that would be cleaner so that we can do the comparison in code ourselves
There was a problem hiding this comment.
Actually, we already should have that version from stdout above, right? I don't think we need to run any python here at all
There was a problem hiding this comment.
executing python --version returns a literal such as Python 3.11.7.
Felt like parsing this into a version number and then comparing added extra complexity than using python itself to validate version.
I can update if that would be easier to follow
There was a problem hiding this comment.
Ah, ok. I guess there are some nuances with it as well (e.g. comparator of version numbers). I'm fine with keeping as-is using python to check
|
Ok, so let's keep the check logic as-is but please address the other comments:
On top of that, please update the comment to give more context on using python to check version (namely complexity of different version values and how they would need to be parsed and compared) |
Add a helper method to validate the python interpreter version. Simply throws an error on a bad version. Notification dialog in diff on top. Align's with Python AIConfig's version requirement of python3.10 and higher ## Testplan Select a python interpreter 3.9, which is less than aiconfig's requirement version of 3.10. https://github.com/lastmile-ai/aiconfig/assets/141073967/4cfcb153-50aa-40fb-9764-54e964d4b1b8
Ankush-lastmile
force-pushed
the
pr1262
branch
from
efd4e79
to
57e9a39
Compare
Can revisit the hardcoded dependency of python |
[vscode installation] 1/n Python version validation
Add a helper method to validate the python interpreter version. Simply throws an error on a bad version. Notification dialog in diff on top.
Align's with Python AIConfig's version requirement of python3.10 and higher
Testplan
Select a python interpreter 3.9, which is less than aiconfig's requirement version of 3.10.
output.mp4
Stack created with Sapling. Best reviewed with ReviewStack.