Skip to content
This repository was archived by the owner on Jul 15, 2025. It is now read-only.

Conversation

@hmf
Copy link
Contributor

@hmf hmf commented Nov 26, 2022

While documenting the API usage for my notes, I had a problem discussed in issue #11. This PR adds the tests I used when the problem was detected.

Besides the tests, I noticed a compilation error that was also corrected by adding a requires module. See java/module-info.java that has this line:

requires java.desktop; // required for java.awt.*

  • Tests pass
  • Appropriate changes to README are included in PR

Copy link
Contributor

@Craigacp Craigacp left a comment

Choose a reason for hiding this comment

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

Looks good apart from the module-info update. We'll fix that another way rather than contaminate the runtime module with things that the tests need.

*/
module org.tensorflow.ndarray {
requires jdk.unsupported; // required by raw buffer implementations using Unsafe
requires java.desktop; // required for java.awt.*
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only used in a test, and I'd prefer to refactor the test to use a serialized byte array than have java.desktop as a module level dependency for the whole of TF.

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 will remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could also supply a module-info in src/test/java which has the extra requires, that should placate the module system and Maven.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Craigacp Got a minimal module-info but needed some adjustments. First, if I stick to the file name module-info.java, compilation fails. If I use the file name module-info.test, then it compiles and executes the tests correctly.

The simplest version that I pushed, only adds that one requirement and also changes the module name to org.tensorflow.ndarray.test. This last change is not necessary. Keeping the same name, works. As per this link, I changed the name.

In the link above, the example seem to have the same entries of the main module repeated in the test module. Should that be done?

In addition to this, they have (for JUnit5):

    requires transitive org.junit.jupiter.engine;
    requires transitive org.junit.jupiter.api;

With these, compilation and testing also work. Want to add these too?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think using a different module name implies that we won't easily be able to do white box testing (i.e. access protected methods for testing). If it does have a different module name it should have a dependency on the org.tensorflow.ndarray module, and either way I think it should have the dependencies on junit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So which is it? Keep the same name or add a dependency?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to use the same name and have Maven patch the module overriding it for the tests. However I've definitely run into issues doing that before, particularly if the test module is depended on elsewhere (which it isn't in this case), so it depends how it goes if you try it. I've not had chance to setup the right environment to test it out locally.

Copy link
Contributor Author

@hmf hmf Nov 30, 2022

Choose a reason for hiding this comment

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

I have kept the same module name. Compilation and testing execute correctly. I could not get the module-info.java file to work, so its named module-info.test. This reference did not help, but may shed light for anyone trying to change this.

Copy link
Contributor

@Craigacp Craigacp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@Craigacp Craigacp merged commit f54dd15 into tensorflow:main Nov 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants