Fix #335 Improve overloaded varargs resolution#425
Conversation
When multiple Java varargs overloads match a Python argument list, rank the candidates by conversion quality before falling back to signature ordering. This preserves the preference for non-varargs overloads added for jython#221 while avoiding last-match or first-match behavior among varargs methods. Add regression coverage for mixed varargs overloads such as insert(String, String...) and insert(String, boolean...), plus boolean varargs cases that previously exposed unstable overload selection. Generated with assistance from Codex.
|
Here is another stab at this issue. I had Codex aid me in generating these changes but I was very validated that it implemented the same structure that I had been trying to wrap my head around. It implemented the scoring mechanism that @Stewori suggested in #336. Given two candidate vararg matches this will compute cost to convert the arguments python types to java types. A PyBoolean to a Java The keys here are There seems to be more activity than normal around JDK25 cleanup. My project has already switched to JDK 25 and while things still work for us with our patched version of 2.7.2 upgrading to an official version 2.7.5 would be a be awesome for us. |
|
Maybe serializable is included because it is the only interface implemented by PyObject. That means, all PyObject subclasses are assignable to Serializable. So, if Serializable is the target it should score somewhat better than just Object - it's more specific. However I don't see why e.g. integer against serializable should score better than integer against boolean. So, that might need some adjustment. Any idea about the performance impact of this PR? The new comparison is more expensive than before. Did you notice a significant difference when running regrtests with or without the PR? |
| || target == Double.class || target == Number.class) { | ||
| return 2; | ||
| } else if (target == Serializable.class) { | ||
| return 10; |
There was a problem hiding this comment.
I think letting serializable score better than a plain Object is justified as all PyObject subtypes implement this interface. So the fit is more specific than against Object. However, I doubt that it should score better than Boolean below. Maybe that's just my opinion. Thoughts?
There was a problem hiding this comment.
I think that gets to the crux of my issue. The old approach was all to happy to convert everything far too many things an array of booleans. Here is an example of the old code misbehaving
# Script evaluated by Jython interpreter
from atst.cs.data import AttributeTable
table = AttributeTable()
table.insert('dbl', 1.0, 2.0, 3.14)
table.insert('str', 'foo', 'bar', 'waz')
table.insert('int', 1, 2, 3)
table.insert('bool', True, False)
table.show('')
Previously produced
ScriptReader Interpreter.dbl: true, true, true
ScriptReader Interpreter.str: foo, bar, waz
ScriptReader Interpreter.int: true, true, true
ScriptReader Interpreter.bool: true, false
After some back and forth with codex it was able to clarify that the issue is a bit of an edge case but given the class:
import java.io.Serializable;
import java.util.Arrays;
public class Example {
public String f(Serializable... values) {
return "Serializable: " + values[0].getClass().getName();
}
public String f(boolean... values) {
return "boolean: " + Arrays.toString(values);
}
}
Invoking Example().f(1) would trigger the boolean path which is less faithful than the serializable path.
I think that given that the non-varargs path doesn't include the special handling of serializable (meaning foo(Serializable s) and foo(boolean b) will prefer the latter it would probably be best to drop the serializable test here. That keep the fixed arg and vararg behavior more consistent and not risk breaking things.
I will remove this Serializable check.
There was a problem hiding this comment.
But if you remove the serializable check, would f(1) in
public String f(Serializable... values) {
return "Serializable: " + values[0].getClass().getName();
}
public String f(Object... values) {
return "boolean: " + Arrays.toString(values);
}
still prefer serializable over object? Maybe yes, because non-primitives are checked elsewhere. Is that so?
There was a problem hiding this comment.
› Back to the question about why Serializable was used as an explict test.
In the case of the portion removed from line 312 in the last commit;
If you remove the serializable check, would f(1) in the example still prefer
serializable of object? Maybe yes, because non-primitives are checked
elsewhere. Is that so?
• Yes, for Serializable vs Object, f(1) would still prefer Serializable even without the
explicit Serializable cost.
If the special case is removed, PyInteger -> Serializable falls through to:
return 50 + precedence(target);
For Serializable.class, precedence() returns 2000, so the cost is 2050.
For Object.class, conversionCost() has an explicit fallback:
target == Object.class -> 3000
So among varargs overloads like:
f(Serializable... s)
f(Object... o)
the integer call still ranks:
Serializable... cost 2050
Object... cost 3000
and Serializable... wins.
The explicit Serializable check matters for comparisons against other legal
conversions with lower fallback cost, especially Boolean:
Boolean 20
Serializable 2050 without special case
So without the check, f(1) would prefer boolean... over Serializable.... It would
not prefer Object... over Serializable....
One nuance: the “non-primitives are checked elsewhere” logic in compare()/compareTo()
is mostly the fixed-signature ordering path. The new varargs cost path does not use
assignability among arbitrary reference types except through getJavaProxy() and
target.isInstance(pyArg). A plain PyInteger is not itself an instance of Serializable; it
only converts to a Java Integer through __tojava__, which is why the explicit
Serializable cost changes varargs ranking.
There was a problem hiding this comment.
Alright, good to know. What do you think of scoring boolean much worse? Since in Python every object implements __nonzero__, it can be used as a boolean. So, it might be justified to score that somewhere closer to the magnitude of other ultra-generalists like Serializable and Object. Perhaps at around 1000 or 1500 or so. What do you think?
There was a problem hiding this comment.
1000 or 1500 isn't sufficient. We'd have to bump it to >2050 for an actual change of ordering. A value like 2500 would change the behavior if you had an overload like foo(boolean) and foo(BigInteger) and you tried to invoke with a PyLong. Should a PyLong map directly to a BigInteger? Maybe, but I don't think that is how the non-varargs resolver will behave and I think that consistency is important.
There was a problem hiding this comment.
In principle, I think PyLong should indeed preferably cast to BigInteger rather than boolean. However, boolean should be preferred over Serializable and Object. Consistency with non-varargs is a fair point though. Is the behavior consistent in the current form of this PR? For PyLong/BigInteger/boolean, it would be good to pinpoint the current behavior (this PR) in vararg vs non-vararg by an example code snippet, perhaps by a test. To avoid relying on guessing for the anticipated consistency.
I already wondered whether the scoring might not also be a better solution for non-vararg resolving, but such changes would probably be too breaking. Perhaps a thought for Jython 3.
There was a problem hiding this comment.
I've added some additional test cases that are just concerned with whether the single arg and var args resolutions match. Those tests exposed one case where previously the varargs and fixed arity behavior didn't match. The fixed arity resolution prefers Object over boolean when the argument is PyFloat. This was addressed by adding a very high cost to PyFloat --> boolean. I kind of think that the fixed-arity decision is wrong but that feels out of scope for these changes. If you'd like I can open a ticket for the fixed arg case.
| if (target == String.class) { | ||
| return 0; | ||
| } else if (target == Serializable.class) { | ||
| return 10; |
There was a problem hiding this comment.
Similar critique as above. Here I'm more confident though. Taking a String for a Character should score way better than for a Serializable.
There was a problem hiding this comment.
I will remove the serializable check as well.
Remove instanceof Serializable checks as they create a difference in behavior between the varargs and fixed args resolution.
I have very little experience running the regrtests and can't really say what the performance impact is. I'm not sure how consistent things the are but the Ubuntu and Windows action runs for this PR took 11:13 and 18:14 respectively. The latest run for master took 11:18 and 17:19. It kind of feels like these changes are probably in the noise but the impact would obviously be felt more on code bases that heavily utilize varargs overloads. The resolution would be most expensive in cases where there are a lot of arguments that all need to be costed and there are multiple candidate overloads. One good thing about the current implementation is that it does not compute the cost of the varargs conversions unless it needs to see if the cost of the current best function is better than the cost of an alternative function. The bad thing about the current implementation is that it does not cache the cost of the current preferred method. So, if you have a function with many overloads and many many arguments and the first checked function was the best option, it will still re-compute the cost for that first function for each subsequent comparison. |
Use new isSequenceVarargs in existing ensureBoxedVarargs() to ensure a single source of truth. Move original comments regarding why a long list of lcasses is being used instead of PySequence to the new method.
Add overload-resolution regression coverage comparing fixed-arity and varargs dispatch for PyInteger, PyLong, and PyFloat against boolean, BigInteger, BigDecimal, and Object overloads. Keep varargs scoring aligned with existing fixed-arity behavior by handling PyLong boolean conversion explicitly, making PyFloat boolean conversion more expensive than Object, and ensuring Object keeps its intended high fallback cost.
|
I believe the initial PR feedback has been addressed. Much thanks to @Stewori for his thoughtful input. |
|
TBH, this change is a bit risky as it may break established behavior and is somewhat experimental. We will discuss and consider whether we can include this so shortly before a release. No promise that it will be included into 2.7.5, but we consider it. |
|
I understand the concern about risk, especially close to a release. I'm trying to understand the expected path forward if this doesn't make 2.7.5. Before the recent activity around JDK 25 compatibility and cleanup work, I wasn't sure whether there would be a 2.7.5 release, or whether effort was primarily focused on a future 3.x release. That uncertainty was part of why I hadn't pursued this more aggressively earlier. There seems to be more contributor and reviewer engagement at the moment. Getting the change into a 2.7.5 release candidate might provide more opportunities for testing and feedback than waiting for a future release cycle. |
|
This is a long thread so I apologize for such a basic question: What part of the resolution problem are you trying to improve? There was a very long discussion in #282 about how that was a partial solution but didn't deal with the "types" issue (i.e., in Java, I can overload based on short, int, long but that really doesn't work in Jython due to the type conversions that happen prior to method selection). Our customers have millions of existing scripts so changes at this level in a patch release seem very dangerous. |
Given Java code with overloaded varargs methods or constructors, jython/python scripts should not tend to everything is truthy and thus anything can match a Prior to the #221 changes that landed in Jython 2.7.3 code like would resolve in our system to One of the things that we landed on later (thanks @Stewori for your thoughtful comments) was to ensure that the behavior was the same for the fixed and variable arity resolution. So if jython |
|
@robertpatrick Indeed I was thinking of you when I exercised care in accepting this PR for 2.7.5. OTOH I think it brings a real improvement and there should be a way to include this into Jython eventually. Maybe the point against 2.7.5 specifically is that this release is crucial as it restores full compatibility with modern Java. Also the move from javax.servlet to jakarta.servlet is a killer-feature forcing many users to update. (@robertpatrick I hope your customers are prepared for that breaking change - otherwise one can put the old javax.servlet-based classes on top of the new Jython in classpath to retain the old behavior.) That said, I acknowledge that @jhubbardnso may also have a business case for adding this change and we do not actually have a census suffrage system based on customer-numbers. If we include this feature after 2.7.5, Robert's customers would never be able to upgrade safely, so it would just postpone the trouble. What about the following idea: It should be fairly easy to condition the new behavior on a flag. So all it would take to cure an issue with the change would be like As a side effect, having such a flag would be convenient for testing or debugging old against new behavior. So I would recommend it for this type of "risky" change already on its own right. We could additionally make it writable via command line option, e.g. There is even a way to config this via a manifest-like file. It would be possible to package into a jar file a file ...
public class MyInitializer implements JythonInitializer {
public void initialize(Properties preProperties, Properties postProperties, String[] argv,
ClassLoader classLoader, ExtensiblePyObjectAdapter adapter)
{
org.python.core.ReflectedArgs.setLegacyMode(True);
}Then, just putting that jar on the classpath along with Jython would set the flag. Yes, you can config Jython via classpath manipulation. (What an intriguing hidden feature, isn't it? Disclaimer: Not my invention this time!) |
Our usage of Jython does not use this portion of the code. However, our last release is already on Jakarta EE 9.1 and the upcoming release (where we will support JDK 25) will be on Jakarta EE 11 so we are way past the javax to jakarta transformation.
I am not sure how intrusive the change is. I would need to test it with our test suite to see if it breaks anything. However, if it does break stuff, I will need to find a way forward.
Definitely not desirable since we would need to do a bunch of mind-numbing shell scripting to handle all the use cases with regards to the |
|
So I see the following options:
@jhubbardnso @robertpatrick @jeff5 @jhubbardnso How would Option 4 sound to you? Would you be willing to implement a flag in this manner? |
|
My preference (in order) would be 2, 3 then 4 😉 If an environment variable can be used to enable the new behavior I should be able to inject it relatively easily into our system. If we go with an opt-in flag (option 4) I think we should work to get it switched to an opt-out flag for whatever comes after 2.7.5. I'll see if I can't mock up the flag option as well as some unit tests to codify what the current behavior is. |
Thx! I guess this option should be added to org.python.core.RegistryKey. Javadoc of the field is the ideal place to document usage, purpose and semantics of the arg and its value.
|
Introduce ReflectedArgs.setLegacyMode(boolean) so callers can restore the previous last-matching-varargs behavior while keeping the improved conversion cost ranking as the normal path. Expand overload regression coverage with dense primitive, numeric, Object, fixed-prefix, and legacy-mode varargs cases. Adjust PyInteger and PyLong conversion costs so ranked varargs dispatch stays aligned with existing fixed-arity overload resolution.
|
@Stewori I think we need to better understand the behavior change to evaluate opt-in vs. opt-out. I am still waiting for JDK 25 support to be merged before I start testing more changes like this one. Any ETA on that? Is this change simply to prefer |
|
@robertpatrick I've gone ahead and pushed the first bit of the work to make the new behavior optional. It is opt out at the moment but I'm open to changing it. (I might prefer opt out for the RC with the plan being to change it to opt-in if people complain. Importantly I've added tests cases to capture the legacy behavior, maybe that will help explain things. The legacy test includes a case where the fixed and variable arity methods resolve differently. This shouldn't change anything when dealing with I still need to support the |
|
@jhubbardnso As long as that type of pattern is all that it is changing, it is probably ok to put it in and allow opt-out. I cannot test until @jeff5 finishes processing #424 |
|
In fact, I believe this change generally won't break any of our code so I'm ok if you merge it without opt-in/opt-out code. |
|
I'm not keen on a change of semantics that means an opt-in/out. Are we going to test and maintain both versions? What if the authors of different modules want different behaviours? The first message of the thread mentions improving the regression tests. (I mean to include both The desired behaviour is something like that means the same as the Java it resembles, but I don't think it can quite, because the same type information is just not there. Sorry, I don't have the time to consider in this in proper detail until #424 is past. |
This is not meant to be an each module chooses what they want...but you already knew that. I am more inclined to just put it in with no opt-out behavior. If it breaks stuff, we either fix it in a follow-up PR or for anyone that cannot tolerate it, they fork the repo and fix it to suit their needs.
Somewhat...it depends on the complexity and the unit test coverage.
In the right hands, AI can infer a lot. Of course, there are ways to help it...like leaving comments in the code to explain why you are doing something a particular way. |
4 participants
When multiple Java varargs overloads match a Python argument list, rank the candidates by conversion quality before falling back to signature ordering. This preserves the preference for non-varargs overloads added for #221 while avoiding last-match or first-match behavior among varargs methods.
Add regression coverage for mixed varargs overloads such as insert(String, String...) and insert(String, boolean...), plus boolean varargs cases that previously exposed unstable overload selection.
Generated with assistance from Codex.