Skip to content
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

Function for converting Python values to an explicit Java type #128

Merged
merged 30 commits into from
Jan 24, 2024

Conversation

rbasralian
Copy link
Contributor

@rbasralian rbasralian commented Jan 10, 2024

  • Fix a bug in cast
  • Add an as_jobj method to convert a Python value to a Java object of the given type. This leverages the existing JPy_AsJObjectWithType method.
  • Update some tests to match current behavior (e.g. returning Byte instead of Integer)
  • More logging during tests
  • Fixed failing hashNegativeOne test
  • Replace incorrect return Py_NONEs with Py_RETURN_NONE (for correct reference counting)

@@ -57,10 +59,19 @@ static PyMethodDef JPy_Functions[] = {
"get_type(name, resolve=True) - Return the Java class with the given name, e.g. 'java.io.File'. "
"Loads the Java class from the JVM if not already done. Optionally avoids resolving the class' methods."},

{"get_type_name", (PyCFunction) JPy_get_type_name, METH_VARARGS,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? You can currently do type(jobj).jclass.getName(). This does involve a boundary crossing though.

If we think it's important to have w/o boundary crossing, and explicitly meant for jobj types, instead of adding jpy top level method, we can attach an attribute to the JPy_JType. From end user perspective, it would look something like:

# Sources from JPy_JType->javaName
type(jobj).jclassname

would be appropriate. See logic in JType_AddClassAttribute.

I'm actually surprised that

type(jobj).__name__

isn't the full classname; https://docs.python.org/3/c-api/typeobj.html#tp-slots hints that that should be tp_name.

And our code has this:

typedef struct JPy_JType
{
...
    // typeObj.tp_name is this type's fully qualified Java name (same as 'javaName' field).
    PyTypeObject typeObj;
    // The Java type name.
    char* javaName;

Something screwy is going on though:

>>> jpy.get_type('java.lang.CharSequence').__name__
'CharSequence'
>>> jpy.get_type('java.math.BigInteger').__name__
'BigInteger'

I'll do some further digging, b/c this seems odd...

Copy link
Member

Choose a reason for hiding this comment

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

https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_name

Looks like

>>> f"{type.__module__}.{type.__name}"

is the way to get the java class name; this doesn't quite work for primitives (not sure what we expect to happen w/ primitives).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think primitives apply — I don't think you can wrap an unboxed primitive value in a JPy_JObj.

I got rid of this method and added type(obj).jclassname.

jpyutil.py Outdated Show resolved Hide resolved
Comment on lines 682 to 684
if (resultObj == NULL) {
return NULL;
}
Copy link
Member

Choose a reason for hiding this comment

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

We should delete the global ref if this happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks — fixed

{"cast", JPy_cast, METH_VARARGS,
"cast(obj, type) - Cast the given Java object to the given Java type (type name or type object). "
"Returns None if the cast is not possible."},

{"as_jobj", JPy_as_jobj, METH_VARARGS,
Copy link
Member

Choose a reason for hiding this comment

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

This is cool - previously, we could do this, it just required explicit java code:

public class MyClass {
  public static MyJavaType identity(MyJavaType x) {
    return x;
  }
}
jobj = jpy.get_type('MyClass').identity(pobj)

I'm assuming these are the semantics you are after?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, basically I had two parallel lists, one of java types and one of python values (in practice strings/numbers), and I wanted to stick the python values into a java Object[] with the given Java type, but numbers I wanted as Long were getting converted to Byte. I could have just used the constructors explicitly but that's not as clean, and it seems worthwhile to expose JType_ConvertPythonToJavaObject anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not too crazy about the name as_jobj, would conv be better?
From what I can gather, I do like that I can now create a org.jpy.PyObject in Python directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm definitely open to changing it. I went with as_jobj because it ultimately calls JPy_AsJObjectWithType, and I thought having 'obj' in the name explicitly could be helpful because conversion in Java is a distinct concept (though a related one) that also involves Java primitives.

@devinrsmith what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think "as" gives off the wrong semantic notion, somewhat akin to zero-cost. While the same could be said about JPy_AsJObjectWithType, that's lower level and not exposed to the end user. Also, it calls down into JType_ConvertPythonToJavaObject.

I'm also not the biggest fan of trying to save a few characters; my vote would be for jpy.convert instead of jpy.conv / jpy.as_jobj.

self.assertEqual(fixture.stringifyObjectArg(my_jcharseq2), 'String(testStr)')


def test_ToObjectConversionTyped(self):
Copy link
Member

Choose a reason for hiding this comment

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

I think trying to exercise some of the other logic from JType_ConvertPythonToJavaObject would be nice; arrays, boolean, char, PyObject, String.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added more tests. The test for converting explicitly to PyObject doesn't run, apparently due to an issue with the classpath

Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

I don't think your changes to the JVOID / JNULL macros match with their indented use cases. While technically the results will be the same, the usage semantics are wrong. I would revert all these changes.

As you've probably noticed, the jpy tests aren't very robust. Ideally, we'd get them to run in GH CI here, but that hasn't been done yet.

Comment on lines +925 to +929
} else if (JPy_IS_CLONG(pyArg) && (*jenv)->IsAssignableFrom(jenv, JPy_Long_JClass, type->classRef)) {
return JType_CreateJavaLongObject(jenv, type, pyArg, objectRef);
} else if (PyFloat_Check(pyArg) && (type == JPy_JObject || ((*jenv)->IsAssignableFrom(jenv, JPy_Double_JClass, type->classRef)))) {
} else if (PyFloat_Check(pyArg) && (type == JPy_JObject || (*jenv)->IsAssignableFrom(jenv, JPy_Double_JClass, type->classRef))) {
return JType_CreateJavaDoubleObject(jenv, type, pyArg, objectRef);
} else if (PyFloat_Check(pyArg) && (type == JPy_JObject || ((*jenv)->IsAssignableFrom(jenv, JPy_Float_JClass, type->classRef)))) {
} else if (PyFloat_Check(pyArg) && (*jenv)->IsAssignableFrom(jenv, JPy_Float_JClass, type->classRef)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you comment on the effect of this? It looks like you removed the JPy_JObject condition from Long / Float, but not Double?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the cases I changed were:

    } else if (JPy_IS_CLONG(pyArg) && (type == JPy_JObject || (*jenv)->IsAssignableFrom(jenv, JPy_Number_JClass, type->classRef))) {
        return JType_CreateJavaNumberFromPythonInt(jenv, type, pyArg, objectRef);
    } else if (JPy_IS_CLONG(pyArg) && ((*jenv)->IsAssignableFrom(jenv, JPy_Integer_JClass, type->classRef))) {
        return JType_CreateJavaIntegerObject(jenv, type, pyArg, objectRef);
    } else if (JPy_IS_CLONG(pyArg) && (type == JPy_JObject || ((*jenv)->IsAssignableFrom(jenv, JPy_Long_JClass, type->classRef)))) {
        return JType_CreateJavaLongObject(jenv, type, pyArg, objectRef);

and:

    } else if (PyFloat_Check(pyArg) && (type == JPy_JObject || ((*jenv)->IsAssignableFrom(jenv, JPy_Double_JClass, type->classRef)))) {
        return JType_CreateJavaDoubleObject(jenv, type, pyArg, objectRef);
    } else if (PyFloat_Check(pyArg) && (type == JPy_JObject || ((*jenv)->IsAssignableFrom(jenv, JPy_Float_JClass, type->classRef)))) {
        return JType_CreateJavaFloatObject(jenv, type, pyArg, objectRef);
    }

in both case, the second type == JPy_JObject test will always be false — for the first case, we will have already landed in JType_CreateJavaNumberFromPythonInt and in the second case we'd hit JType_CreateJavaDoubleObject.

@@ -451,7 +457,7 @@ PyObject* JPy_create_jvm(PyObject* self, PyObject* args, PyObject* kwds)
if (JPy_JVM != NULL) {
JPy_DIAG_PRINT(JPy_DIAG_F_JVM + JPy_DIAG_F_ERR, "JPy_create_jvm: WARNING: Java VM is already running.\n");
JPy_DECREF(options);
return Py_BuildValue("");
return JPy_FROM_JVOID();
Copy link
Member

Choose a reason for hiding this comment

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

JPy_FROM_JVOID is meant to be used in combination with void.class type; it's not appropriate to use it in these contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced the handful of Py_BuildValue()s I changed with a new JPy_PY_NONE()

return NULL;
}

if (obj == Py_None) {
return Py_BuildValue("");
return JPy_FROM_JNULL();
Copy link
Member

Choose a reason for hiding this comment

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

This is not appropriate to use here unless we are converting from a null java object.

} else {
return Py_BuildValue("");
return JPy_FROM_JNULL();
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

*/
// System.out.println(obj.getObjectValue());

// assertEquals(-1, obj.getIntValue()); // TODO: should this be 'result.getIntValue()'?
Copy link
Member

Choose a reason for hiding this comment

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

Yes; this should be result.getIntValue()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks — updated & reenabled the test


def test_AsJobjToPyObject(self):
print('Starting test_AsJobjToPyObject')
# TODO: "Java class 'org.jpy.PyObject' not found"???
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense; PyObject is only necessary if the user needs to reference python from java. At least with this test scaffolding, the jpy jar is not on the classpath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok — I moved this to a separate file (jpy_typeconv_test_pyobj.py).

Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

LGTM. Adding Jianfeng for final pass.

Copy link
Contributor

@jmao-denver jmao-denver left a comment

Choose a reason for hiding this comment

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

The changes LGTM, not sure if I like the name very much.

{"cast", JPy_cast, METH_VARARGS,
"cast(obj, type) - Cast the given Java object to the given Java type (type name or type object). "
"Returns None if the cast is not possible."},

{"as_jobj", JPy_as_jobj, METH_VARARGS,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not too crazy about the name as_jobj, would conv be better?
From what I can gather, I do like that I can now create a org.jpy.PyObject in Python directly.

Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@jmao-denver jmao-denver left a comment

Choose a reason for hiding this comment

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

I am happy with the name change.

@devinrsmith devinrsmith merged commit 1331a91 into jpy-consortium:master Jan 24, 2024
27 checks passed
@rbasralian rbasralian deleted the raffi_obj_conv branch January 24, 2024 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants