-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Add JAR files as runtime dependencies to Python package. #24
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
base: main
Are you sure you want to change the base?
Conversation
e31f343 to
ff5f421
Compare
|
note: in order to run a specific test pypi version in NeqsimAPI: (optional, try to remove the whole .venv aswell: |
5ba22e0 to
6100c02
Compare
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.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
117b77c to
ce0d3db
Compare
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.
Pull request overview
Copilot reviewed 15 out of 17 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/publish.yaml
Outdated
| - name: Publish package to TestPyPI | ||
| run: hatch publish -r test | ||
| env: | ||
| HATCH_INDEX_USER: __token__ | ||
| HATCH_INDEX_AUTH: ${{ secrets.TEST_PYPI_TOKEN_MAOHE }} |
Copilot
AI
Dec 5, 2025
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.
Breaking change: Publishing to TestPyPI instead of PyPI. This appears to be a testing/development change that shouldn't be in a production release. The original code published to PyPI, but now it publishes to TestPyPI with a personal token (TEST_PYPI_TOKEN_MAOHE). This should either be reverted to publish to PyPI, or this should be documented as a temporary testing workflow.
| - name: Publish package to TestPyPI | |
| run: hatch publish -r test | |
| env: | |
| HATCH_INDEX_USER: __token__ | |
| HATCH_INDEX_AUTH: ${{ secrets.TEST_PYPI_TOKEN_MAOHE }} | |
| - name: Publish package to PyPI | |
| run: hatch publish | |
| env: | |
| HATCH_INDEX_USER: __token__ | |
| HATCH_INDEX_AUTH: ${{ secrets.PYPI_TOKEN }} |
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.
We will change this before merging!
| if JPYPE_AVAILABLE and jpype and not jpype.isJVMStarted(): | ||
| # We need to start the JVM before importing the neqsim package | ||
| try: | ||
| jar_path = get_neqsim_jar_path(jpype.getJVMVersion()) | ||
| jpype.startJVM(classpath=[jar_path]) | ||
|
|
||
| return str(jar_path) | ||
| import jpype.imports | ||
|
|
||
| # This is the java package, added to the python scope by "jpype.imports" | ||
| neqsim = jpype.JPackage("neqsim") | ||
| except Exception as e: | ||
| # JVM Start failed, handle gracefully | ||
| import warnings | ||
|
|
||
| if not jpype.isJVMStarted(): | ||
| jpype.startJVM() | ||
| jar_path = get_neqsim_jar_path(jpype.getJVMVersion()) | ||
| jpype.addClassPath(jar_path) | ||
| import jpype.imports # noqa | ||
| warnings.warn(f"Failed to initialize JVM: {e}. NeqSim functionality will not be available.", stacklevel=2) |
Copilot
AI
Dec 5, 2025
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.
JVM lifecycle issue: The code checks not jpype.isJVMStarted() to start the JVM, but JPype JVM can only be started once per process. If the JVM start fails (line 50), subsequent imports of this module won't retry starting the JVM since the check happens at module import time. Also, if another part of the code starts the JVM before this module is imported, the JAR won't be added to the classpath. Consider handling these edge cases or documenting the expected usage pattern.
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.
This is the only place we start the JVM
Aims to fix problem where jneqsim package became too large