Extract tf.function decorator keyword arguments #144

tatianacv · 2023-02-10T21:45:57Z

Fixes #136 .

Commits

I slowed down and took a deep breadth before committing. Otherwise, I will start a pristine branch and reissue this pull request.
Before committing my changes, I checked my working directory carefully. I only staged the changes I wanted and ensured it was of the utmost quality.
I used git diff and git status to check my work carefully before committing.
I completely understand what I am committing and why.
I completely understand why things are the way they are.
The change I am proposing makes complete sense to me.
For the most part, the changes in this pull request correspond to the problem or task I am solving or performing. In other words, only the changes that are supposed to be there are the ones included in this pull request.

Expectations

Extract tf.function decorator keyword parameter values. We are obtaining the decorator’s argument literal values. We throw an exception when we find something we cannot process, like for example, non-literal values.

Testing

If this is a bug fix, I was able to reproduce the problem locally before I made any changes (can use git stash to temporarily reset the working directory).
When I made (or put back) my changes, the problem I reproduced above was fixed.

I added tests for many possible values that could arise for each tf.function arguments. I used the tf.function documentation to understand and make tests for different possible values. I also added test to make sure that for when we are not working with literals, we throw an exception.

Final Checks

The changes I am proposing meet the expectations I have described above.
I have performed a self-review of my code.
I have commented my code, particularly in hard-to-understand areas.
I have made corresponding changes to the documentation if applicable.
I have added tests that prove my fix is effective or that my feature works if applicable.
New and existing unit tests pass locally with my changes.

tatianacv · 2023-03-10T19:42:22Z

Also add a URL for each parameter using an HTML anchor for where it is described in the docs for the specific version (permalink) we are considering.

There is no URL for each tf.function parameter. The most specific permalink would be https://www.tensorflow.org/versions/r2.9/api_docs/python/tf/function#args

khatchad · 2023-03-21T00:42:03Z

Also add a URL for each parameter using an HTML anchor for where it is described in the docs for the specific version (permalink) we are considering.

There is no URL for each tf.function parameter. The most specific permalink would be https://www.tensorflow.org/versions/r2.9/api_docs/python/tf/function#args

Yeah? Like this one? https://www.tensorflow.org/versions/r2.9/api_docs/python/tf/function#input_signature

edu.cuny.hunter.hybridize.core/src/edu/cuny/hunter/hybridize/core/analysis/Function.java

khatchad

Getting there.

khatchad · 2023-03-21T00:48:48Z

edu.cuny.hunter.hybridize.core/src/edu/cuny/hunter/hybridize/core/analysis/Function.java


 		/**
 		 * Value of this {@link Function}'s {@link decoratorsType} parameter experimental_autograph_options. The values could be an optional
 		 * tuple or value of tf.autograph.experimental.Feature values or None.
 		 */
-		private String experimentalAutographOptionsParamValue;
+		private String experimentalAutographOptionsParam;


String isn't a good type to represent these values. The docs even say, "this enumeration represents optional conversion options.". That's a hint at what Java (also in C++) type can be used here.

P.S. It's unfortunate that even the example there is using a decorator value from a variable.

This could be an enumeration or tuple of enumerations (https://www.tensorflow.org/versions/r2.9/api_docs/python/tf/function#experimental_autograph_options). How can this be represented?

For example:
https://github.com/tatianacv/Hybridize-Functions-Refactoring/blob/9163368a396e3108700da9aa211a27f0f1d47160/edu.cuny.hunter.hybridize.tests/resources/HybridizeFunction/testDecoratorArguments16/in/A.py#L1-L10
vs
https://github.com/tatianacv/Hybridize-Functions-Refactoring/blob/9163368a396e3108700da9aa211a27f0f1d47160/edu.cuny.hunter.hybridize.tests/resources/HybridizeFunction/testDecoratorArguments18/in/A.py#L1-L10

khatchad · 2023-03-21T00:50:47Z

edu.cuny.hunter.hybridize.core/src/edu/cuny/hunter/hybridize/core/analysis/Function.java


 		/**
 		 * Value of this {@link Function}'s {@link decoratorsType} parameter experimental_autograph_options. The values could be an optional
 		 * tuple or value of tf.autograph.experimental.Feature values or None.
 		 */
-		private String experimentalAutographOptionsParamValue;
+		private String experimentalAutographOptionsParam;

 		/**
 		 * Value of this {@link Function}'s {@link decoratorsType} parameter experimental_implements. The value could be None or a name of a
 		 * "known" function this implements.


Add an example value. This can be in HTML using <code>...</code>.

khatchad · 2023-03-21T00:53:02Z

edu.cuny.hunter.hybridize.core/src/edu/cuny/hunter/hybridize/core/analysis/Function.java

 		 */
-		private String inputSignatureParamValue;
+		private ArrayList<TensorSpec> inputSignatureParam;


Why does this need to specifically be an ArrayList? Is it important that the list be contiguous in memory (this is not a question).

Changed to List

edu.cuny.hunter.hybridize.core/src/edu/cuny/hunter/hybridize/core/analysis/Function.java

khatchad · 2023-03-21T00:58:21Z

edu.cuny.hunter.hybridize.core/src/edu/cuny/hunter/hybridize/core/analysis/Function.java

@@ -204,8 +207,13 @@ public HybridizationParameters(IProgressMonitor monitor) throws BadLocationExcep
 								// Example of value: True, False, None
 								if (keyword.value instanceof Name) {
 									Name value = (Name) keyword.value;
-									if (value.id == "True" || value.id == "False" || value.id == "None") // Checking only literals
-										this.jitCompileParamValue = value.id;
+									if (value.id == "True") // Checking only literals


Per our conversations, we need to be able to tell when a literal value is not used. Thus, we need an exception thrown when a variable is used. Is that what is happening below (this is a question).

khatchad · 2023-03-21T01:00:45Z

edu.cuny.hunter.hybridize.core/src/edu/cuny/hunter/hybridize/core/analysis/TensorSpec.java

+
+		TensorSpec tensor = (TensorSpec) tensorObject;
+
+		return shape.equals(tensor.shape) && dtype.equals(tensor.dtype);


Why are hashCode and equals being overridden for this class? Are you comparing these or putting them into a Set (this is a question)?

It is overridden because, in our tests, I check am checking that the list of tensorspec that is generated is the same one as the one provided.

https://github.com/tatianacv/Hybridize-Functions-Refactoring/blob/9163368a396e3108700da9aa211a27f0f1d47160/edu.cuny.hunter.hybridize.tests/test%20cases/edu/cuny/hunter/hybridize/tests/HybridizeFunctionRefactoringTest.java#L920-L929

khatchad · 2023-03-21T01:01:40Z

edu.cuny.hunter.hybridize.core/src/edu/cuny/hunter/hybridize/core/analysis/TensorSpec.java

-	 * True if the {@link TensorSpec} is using keyword arguments for the type.
-	 */
-	private boolean dtypeKeyword;
-
 	public TensorSpec() {
 		this.shape = "";


Is there more useful representation for a shape than a String?

It is a List

khatchad · 2023-03-21T01:03:03Z

edu.cuny.hunter.hybridize.core/src/edu/cuny/hunter/hybridize/core/analysis/TensorSpec.java

-	 * True if the {@link TensorSpec} is using keyword arguments for the type.
-	 */
-	private boolean dtypeKeyword;
-
 	public TensorSpec() {
 		this.shape = "";
 		this.dtype = "";


Same with dtype. Aren't dtypes finite sets? Or, does it take any arbitrary type (this is a question).

I believe these are the accepted types https://github.com/tensorflow/tensorflow/blob/a5ed5f39b675a1c6f315e0caf3ad4b38478fa571/tensorflow/python/framework/dtypes.py#L518-L542

https://www.tensorflow.org/api_docs/python/tf/dtypes

khatchad · 2023-03-21T01:03:47Z

...idize.tests/test cases/edu/cuny/hunter/hybridize/tests/HybridizeFunctionRefactoringTest.java

@@ -845,7 +846,7 @@ public void testDecoratorArguments() throws Exception {
 		if (!args.hasFuncParam() && args.hasInputSignatureParam() & !args.hasAutoGraphParam() && !args.hasJitCompileParam()
 				&& !args.hasReduceRetracingParam() && !args.hasExperimentalImplementsParam() && !args.hasExperimentalAutographOptParam()
 				&& !args.hasExperimentalFollowTypeHintsParam())
-			assertEquals("None", args.getInputSignatureArg());
+			assertEquals(null, args.getInputSignatureArg());


https://junit.org/junit4/javadoc/4.13/org/junit/Assert.html#assertNull(java.lang.Object)

tatianacv · 2023-03-22T02:54:33Z

Also add a URL for each parameter using an HTML anchor for where it is described in the docs for the specific version (permalink) we are considering.

There is no URL for each tf.function parameter. The most specific permalink would be https://www.tensorflow.org/versions/r2.9/api_docs/python/tf/function#args

Yeah? Like this one? https://www.tensorflow.org/versions/r2.9/api_docs/python/tf/function#input_signature

Thanks! I didn't know I could do that. I didn't see the option of getting the specific link on the site; I thought I couldn't. Thanks so letting me know.

…github.com/tatianacv/Hybridize-Functions-Refactoring into 136-extract-tffunction-decorator-arguments

khatchad · 2023-03-28T18:53:21Z

Also add a URL for each parameter using an HTML anchor for where it is described in the docs for the specific version (permalink) we are considering.

There is no URL for each tf.function parameter. The most specific permalink would be https://www.tensorflow.org/versions/r2.9/api_docs/python/tf/function#args

Yeah? Like this one? https://www.tensorflow.org/versions/r2.9/api_docs/python/tf/function#input_signature

Thanks! I didn't know I could do that. I didn't see the option of getting the specific link on the site; I thought I couldn't. Thanks so letting me know.

It's in the HTML.

khatchad · 2023-03-28T18:57:04Z

Also add a URL for each parameter using an HTML anchor for where it is described in the docs for the specific version (permalink) we are considering.

There is no URL for each tf.function parameter. The most specific permalink would be https://www.tensorflow.org/versions/r2.9/api_docs/python/tf/function#args

Yeah? Like this one? https://www.tensorflow.org/versions/r2.9/api_docs/python/tf/function#input_signature

Thanks! I didn't know I could do that. I didn't see the option of getting the specific link on the site; I thought I couldn't. Thanks so letting me know.

https://khatchad.commons.gc.cuny.edu/research/background/#link-6074

khatchad

Javadoc comments are outdated. Many of them say return String but the methods don't return String. Because of this reason, it doesn't make sense to to use types in the comments. The comments are used to explain what the code doesn't tell us. We can see from the method signature what the return type is. The javadoc should explain the concept of what is being returned.

khatchad · 2023-03-28T19:47:49Z

edu.cuny.hunter.hybridize.core/src/edu/cuny/hunter/hybridize/core/analysis/Function.java

@@ -38,10 +39,14 @@
 */
 public class Function extends RefactorableProgramEntity {

+	public enum TfAutographExperimentalFeature {


Need a javadoc comment here explaining what this is and a URL for more info.

khatchad · 2023-03-28T19:48:36Z

edu.cuny.hunter.hybridize.core/src/edu/cuny/hunter/hybridize/core/analysis/Function.java

@@ -38,10 +39,14 @@
 */
 public class Function extends RefactorableProgramEntity {

+	public enum TfAutographExperimentalFeature {
+		ALL, AUTO_CONTROL_DEPS, ASSERT_STATEMENTS, BUILTIN_FUNCTIONS, EQUALITY_OPERATORS, LISTS, NAME_SCOPES


What do each of these mean? What is the URL where we can find out more info?

Added an URL in the javadoc

khatchad · 2023-03-28T19:50:21Z

edu.cuny.hunter.hybridize.core/src/edu/cuny/hunter/hybridize/core/analysis/Function.java

@@ -101,7 +107,7 @@ public class HybridizationParameters {
 		 * Value of this {@link Function}'s {@link decoratorsType} parameter input_signature. The value could be None, or a possibly nested
 		 * sequence of tf.TensorSpec objects specifying the shapes and dtypes of the Tensors that will be supplied to this function.
 		 */
-		private ArrayList<TensorSpec> inputSignatureParam;
+		private java.util.List<TensorSpec> inputSignatureParam;


Why not just List?

Why use List? Is ordering important? Can you have duplicates?

We already have another import for List (org.python.pydev.parser.jython.ast.List). Therefore, I had to specify which List it was by doing java.util.List<TensorSpec>.

Order is important, and we can have duplicates.

khatchad · 2023-03-28T19:56:47Z

edu.cuny.hunter.hybridize.core/src/edu/cuny/hunter/hybridize/core/analysis/Function.java

-			int count = 0;
-			String tempString = "";
+		private java.util.List<Integer> processTupleOrListForShape(exprType[] exprTupleOrList) {
+			java.util.List<Integer> shape = new ArrayList<>();


Javadoc says it's returning a String. Is that still true?

Why not just List?

I don't know how a list of integers represents tensor shape. Why a list? Doesn't a tensor shape just have two integers?

We already have another import for List (org.python.pydev.parser.jython.ast.List). Therefore, I had to specify which List it was by doing java.util.List. It is a list of dimensions (https://www.tensorflow.org/versions/r2.9/api_docs/python/tf/TensorShape) which can be integers or None.

khatchad · 2023-03-28T20:00:32Z

A lot of problems here. The build is failing, and I'm unsure whether the data types make any sense. There also so many comments now that it is difficult to read the code, and the comments are getting out-of-sync with the code. Comments are needed for things the code doesn't tell us.

…e type

tatianacv added 30 commits January 31, 2023 00:21

update

39c7fb5

progress

f21e423

Progress

747d5c5

Progress

8f8c0b1

Progress

4ffa27a

Progress

21a8004

Progress

89e4dd0

Progress

b6f4e9c

adding more testing

b9c8107

Fixing trailing whitespace of tests

d6b110c

Fix to test

26fd8f1

adding other params

9a4cb62

Progress

d0d0875

Modifying test resource

015965a

Modifying test resource

6102351

Renaming variable

a62af55

Formatting

f7a9373

Adding exceptions

c9ee308

Add more info to exception

0ab5892

Adding new line to test resource

db5147f

Restructuring exceptions

b9bd9be

Adding new line to test resource

9db5396

Fixing variable name

ec84857

Progress

74b4bc4

Removing unnecessary else

e12fb16

Progress

7aef65b

Update

9f49fea

Progress

279089b

Progress

ef5d5bb

progress

64bd2b6

tatianacv added 3 commits March 9, 2023 17:15

Restructure

247e1d6

Remove redundancy

9b764d9

Restructuring

bc3ff06

tatianacv and others added 4 commits March 10, 2023 17:12

Adding more information

cb23e2f

update

79d0ff3

Merge branch 'main' into 136-extract-tffunction-decorator-arguments

2c93c3f

Merge branch 'main' into 136-extract-tffunction-decorator-arguments

e690e4f

khatchad reviewed Mar 21, 2023

View reviewed changes

edu.cuny.hunter.hybridize.core/src/edu/cuny/hunter/hybridize/core/analysis/Function.java Show resolved Hide resolved

khatchad requested changes Mar 21, 2023

View reviewed changes

Merge branch 'main' into 136-extract-tffunction-decorator-arguments

bc534a4

tatianacv and others added 9 commits March 22, 2023 09:58

Fixing asserts

9163368

Merge branch 'main' into 136-extract-tffunction-decorator-arguments

09d52cb

Progress

d845f0a

progress

6a13a5e

Progress

4c79a5a

Merge branch '136-extract-tffunction-decorator-arguments' of https://…

720bba7

…github.com/tatianacv/Hybridize-Functions-Refactoring into 136-extract-tffunction-decorator-arguments

Changing dtype

5d63094

fixing comments

0c345f0

Merge branch 'main' into 136-extract-tffunction-decorator-arguments

44b8b7b

khatchad requested changes Mar 28, 2023

View reviewed changes

tatianacv added 4 commits March 31, 2023 15:30

Merge branch 'main' into 136-extract-tffunction-decorator-arguments

c5b576b

Remove unnecessary comments, update comments, change tensorspecs shap…

f2238cf

…e type

Adding another test

45bcfd2

remove file

a8ddc42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract tf.function decorator keyword arguments #144

Extract tf.function decorator keyword arguments #144

tatianacv commented Feb 10, 2023 •

edited

Loading

tatianacv commented Mar 10, 2023

khatchad commented Mar 21, 2023

khatchad left a comment

khatchad Mar 21, 2023

tatianacv Mar 22, 2023

tatianacv Mar 22, 2023

khatchad Mar 21, 2023

tatianacv Mar 31, 2023

khatchad Mar 21, 2023

tatianacv Mar 31, 2023

khatchad Mar 21, 2023

tatianacv Mar 22, 2023

khatchad Mar 21, 2023

tatianacv Mar 22, 2023

khatchad Mar 21, 2023

tatianacv Mar 31, 2023

khatchad Mar 21, 2023

tatianacv Mar 22, 2023 •

edited

Loading

khatchad Mar 21, 2023

tatianacv Mar 22, 2023

tatianacv commented Mar 22, 2023

khatchad commented Mar 28, 2023

khatchad commented Mar 28, 2023

khatchad left a comment

khatchad Mar 28, 2023

tatianacv Mar 31, 2023

khatchad Mar 28, 2023

tatianacv Mar 31, 2023

khatchad Mar 28, 2023

tatianacv Mar 31, 2023

khatchad Mar 28, 2023

tatianacv Mar 31, 2023

khatchad commented Mar 28, 2023


		TensorSpec tensor = (TensorSpec) tensorObject;

		return shape.equals(tensor.shape) && dtype.equals(tensor.dtype);

Extract tf.function decorator keyword arguments #144

Are you sure you want to change the base?

Extract tf.function decorator keyword arguments #144

Conversation

tatianacv commented Feb 10, 2023 • edited Loading

Commits

Expectations

Testing

Final Checks

tatianacv commented Mar 10, 2023

khatchad commented Mar 21, 2023

khatchad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tatianacv Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tatianacv commented Mar 22, 2023

khatchad commented Mar 28, 2023

khatchad commented Mar 28, 2023

khatchad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khatchad commented Mar 28, 2023

tatianacv commented Feb 10, 2023 •

edited

Loading

tatianacv Mar 22, 2023 •

edited

Loading