Add Devicelist method to Tensorflow.java #171
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
|
Building everything with "mvn install" will run the tests, yes. Any issues
with the build?
|
| @@ -103,6 +102,22 @@ private static OpList libraryOpList(TF_Library handle) { | |||
| } | |||
| } | |||
|
|
|||
| public static List<DeviceSpec> listDevices(Optional<DeviceSpec.DeviceType> deviceType, TFE_Context ctx) { | |||
There was a problem hiding this comment.
You shouldn't expose TFE_Context at the public level, these classes are generated from the C API and we always wrapped them up in public endpoints for more flexibility and scalability. So in this case, I suggest maybe to create an EagerSession.Context static nested class that encapsulates a TFE_Context?
| deviceList.add(devSpec); | ||
| } | ||
| TF_DeleteDeviceList(devices); | ||
| if(deviceType.isPresent()) return deviceList; |
There was a problem hiding this comment.
Just general comment, be careful to format your code according to Google Java Style Guide, I saw a bunch of missing spaces in the code that will fail the lint checks to pass when enabled.
| } | ||
| TF_DeleteDeviceList(devices); | ||
| if(deviceType.isPresent()) return deviceList; | ||
| return deviceList.stream().filter(d -> d.deviceType().equals(deviceType.get())).collect(Collectors.toList()); |
There was a problem hiding this comment.
Java streams tends to be slower than simple for loops, the overhead is even worst when manipulating such a small list of objects. While I don't think this method needs to be time-critical, I would suggest to take the simple/faster route here (just my two cents).
|
Thanks for the contribution @tomburke-rse , Like @saudet said a simple |
|
Thanks for alle the advice everyone, I'll add them asap. |
|
|
1 similar comment
|
|
|
Like I mentioned, it is the same error as in the pipeline here, specifically only for the TensorFlowTest class. |
|
Just throwing ideas here, I've personally never tried it with OpenJ9, can you check if you have the same error with a standard OpenJDK version? Also, can we check if the test that crashes is yours by commenting out the custom op library test? |
| import org.tensorflow.internal.c_api.TF_Buffer; | ||
| import org.tensorflow.internal.c_api.TF_Library; | ||
| import org.tensorflow.internal.c_api.TF_Status; | ||
| import org.tensorflow.internal.c_api.*; |
There was a problem hiding this comment.
No star imports in the main classes.
|
My own test is indeed the problem. Not sure why, but I will figure out the problem in the next days. Still a weird error. |
|
I've seen this before, when the native process has a process killing error, since the process dies maven thinks it's a fork error. It should generate a dump file somewhere in the project with more info and the stacktrace. |
|
@tomburke-rse , are you unblocked now? |
This is a draft PR for the list_devices functionality.
Unfortunately, I could not run the tests at all and would highly appreciate any help in setting up the project.
Do I need to build from source to run the tests?
Any remarks and improvements are welcome.