Skip to content

Fix tensor dimensions for Jython 3 #261

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

khatchad
Copy link
Collaborator

@khatchad khatchad commented Jun 13, 2025

Fix #42 and fix #147 and fix #165 and fix #164 and fix #166.

  1. Add logging and comments.
  2. Add workarounds for Tensor type propagation stops upon tensor shape mismatch #195.
  3. The new API being called from Jython 3 now needs new dependencies. See Jython3 installation is fragile wrt dependencies #258.

@khatchad khatchad self-assigned this Jun 13, 2025
@khatchad khatchad added bug Something isn't working parsing jython labels Jun 13, 2025
@khatchad khatchad requested review from msridhar and juliandolby June 13, 2025 19:06
@khatchad khatchad marked this pull request as ready for review June 13, 2025 19:06
@khatchad khatchad requested a review from Copilot June 13, 2025 19:06
Copilot

This comment was marked as outdated.

Copy link

codecov bot commented Jun 13, 2025

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 58.56%. Comparing base (a9a73d7) to head (2903750).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...com/ibm/wala/cast/python/loader/Python3Loader.java 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #261      +/-   ##
============================================
+ Coverage     55.01%   58.56%   +3.55%     
- Complexity      502      517      +15     
============================================
  Files           109      109              
  Lines          7663     7265     -398     
  Branches        827      827              
============================================
+ Hits           4216     4255      +39     
+ Misses         3210     2834     -376     
+ Partials        237      176      -61     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@khatchad khatchad enabled auto-merge (squash) June 13, 2025 19:10
@khatchad khatchad disabled auto-merge June 13, 2025 19:18
@khatchad khatchad enabled auto-merge (squash) June 13, 2025 19:18
public void testDecoratedMethod2() throws ClassHierarchyException, CancelException, IOException {
// NOTE: Change to 0, 0 once https://github.com/wala/ML/issues/147 is fixed.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of 0,0, we are expecting an assertion error.

public void testDecoratedMethod3() throws ClassHierarchyException, CancelException, IOException {
// NOTE: Change to 0, 0 once https://github.com/wala/ML/issues/147 is fixed.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of 0,0, we are expecting an assertion error.

public void testDecoratedMethod10() throws ClassHierarchyException, CancelException, IOException {
// NOTE: Change to 0, 0 once https://github.com/wala/ML/issues/147 is fixed.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of 0,0, we are expecting an assertion error.

public void testDecoratedMethod13() throws ClassHierarchyException, CancelException, IOException {
// NOTE: Change to 0, 0 once https://github.com/wala/ML/issues/147 is fixed.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of 0,0, we are expecting an assertion error.

@msridhar
Copy link
Member

@khatchad lost track of this one is it still waiting on a review?

@khatchad
Copy link
Collaborator Author

@khatchad lost track of this one is it still waiting on a review?

@msridhar Yes. Thanks!

@khatchad khatchad requested a review from Copilot June 25, 2025 20:45
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the project to support Jython 3 by fixing tensor dimension handling, adding new dependencies and logging, and introducing workarounds for open issues.

  • Added required test dependencies (asm-all, jnr-constants)
  • Renamed Jython modules/artifacts to use jython3
  • Enhanced Python3Loader with logging and syntax‐error handling; simplified Python3Interpreter initialization

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
com.ibm.wala.cast.python.test/pom.xml Added dependencies for ASM and JNR constants
com.ibm.wala.cast.python.ml/pom.xml Renamed jython dependency to jython3
com.ibm.wala.cast.python.ml.test/source/com/ibm/wala/cast/python/ml/test/TestNeuroImageExamples.java Contains unresolved merge conflict markers
com.ibm.wala.cast.python.ml.test/pom.xml Updated test artifact to jython3.test
com.ibm.wala.cast.python.jython3/source/com/ibm/wala/cast/python/util/Python3Interpreter.java Removed broad exception catch; lazy‐init now unguarded
com.ibm.wala.cast.python.jython3/source/com/ibm/wala/cast/python/loader/Python3Loader.java Added logger, syntax‐error catch, and info‐level tracing
Comments suppressed due to low confidence (3)

com.ibm.wala.cast.python.ml.test/source/com/ibm/wala/cast/python/ml/test/TestNeuroImageExamples.java:24

  • Unresolved merge conflict markers are present here. Please remove the conflict regions and ensure the intended tensor-dimension logic is finalized.
<<<<<<< contrib_jython3_tensor_dims

com.ibm.wala.cast.python.jython3/source/com/ibm/wala/cast/python/util/Python3Interpreter.java:10

  • Lazy initialization of the static interp is not thread-safe. Consider synchronizing this method or using a thread-safe lazy holder to prevent race conditions.
    if (interp == null) {

com.ibm.wala.cast.python.jython3/source/com/ibm/wala/cast/python/loader/Python3Loader.java:105

  • [nitpick] Frequent logging in tight loops may degrade performance. Consider adjusting the log level or guarding this call if it's not needed in production.
                logger.info(() -> "Evaluating: " + s);

@@ -51,6 +51,14 @@
<groupId>junit</groupId>
<artifactId>junit</artifactId>
</dependency>
<dependency>
<groupId>org.ow2.asm</groupId>
<artifactId>asm-all</artifactId>
Copy link
Preview

Copilot AI Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new dependency doesn't specify a <version>. Please add a version or ensure it's managed by a parent POM to avoid build instability.

Suggested change
<artifactId>asm-all</artifactId>
<artifactId>asm-all</artifactId>
<version>5.2</version>

Copilot uses AI. Check for mistakes.

Copy link
Member

@msridhar msridhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nitpick

@@ -94,15 +101,25 @@ public ConstantFoldingRewriter createCAstRewriter(CAst ast) {
return new ConstantFoldingRewriter(ast) {
@Override
protected Object eval(CAstOperator op, Object lhs, Object rhs) {
String s = lhs + " " + op.getValue() + " " + rhs;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could declare s inside the lambda body so it's only computed if logging is enabled

@msridhar msridhar disabled auto-merge June 28, 2025 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working jython parsing
Projects
None yet
2 participants