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

Process positional arguments #134

Open
wants to merge 61 commits into
base: main
Choose a base branch
from
Open

Conversation

tatianacv
Copy link
Member

@tatianacv tatianacv commented Jan 9, 2023

Fixes #108.

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

We have added code to be able to parse through the positional arguments. In this PR, there is restructuring of the method getDeclaringModuleName() in Util.java from the analysis plugin. There is a new method getDeclaringDefinitionName() using some extracted parts of the getDeclaringModuleName() to be able to access the definition of the decorator so we can get the correct ordering of the decorator’s arguments for the positionals. Then, in the Function->HybridizationParameters class, we added code to get the tf.function definition for the user's TF version. Then, depending on the position we were parsing, we would link the definition to the argument getting to get the resulting tf.function parameter.

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.

Contain tests for each tf.function parameter, test for a mix of positional and keyword, test for default values (to know we are counting them as existing), test for no parameters.

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.

* Test for #106. This tests whether we can parse one tf.function positional argument.
*/
@Test
public void testPositionalParameters() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

We need to know what we are doing with the default values. Why are those special?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment explaining this in the test code.

Copy link
Member

Choose a reason for hiding this comment

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

What test code?

Copy link
Member Author

Choose a reason for hiding this comment

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

In testPositionalParameters12

Copy link
Member

Choose a reason for hiding this comment

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

Why in testPositionalParameters12?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because that test handles the default values.

@khatchad khatchad disabled auto-merge March 8, 2023 20:40
Set<Definition> potentialDeclaringDefitinion = Util.getDeclaringDefinition(selection,
Function.this.containingModuleName, Function.this.containingFile, Function.this.nature, monitor);
if (potentialDeclaringDefitinion.size() == 1)
declaringDefinition = potentialDeclaringDefitinion.iterator().next();
Copy link
Member

Choose a reason for hiding this comment

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

Why is there not an else case? I'm not seeing the logic here. Can be greatly helped with some comments in the code of the case analysis.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added comments for this conditional.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know what variable would be null and why. Rewrite the logic to handle the else case here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have rewritten so we handle the else case here

/**
* Get the name of the module defining the entity described in the given {@link PySelection}.
* Get the set of potential declaring definitions of the entity described in the given {@link PySelection}.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand. How can there be multiple definitions of the same thing?

Copy link
Member

@khatchad khatchad left a comment

Choose a reason for hiding this comment

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

Please do not resolve the comments I make. I will resolve them when I review them. Thanks.

Set<Definition> potentialDeclaringDefitinion = Util.getDeclaringDefinition(selection,
Function.this.containingModuleName, Function.this.containingFile, Function.this.nature, monitor);
if (potentialDeclaringDefitinion.size() == 1)
declaringDefinition = potentialDeclaringDefitinion.iterator().next();
Copy link
Member

Choose a reason for hiding this comment

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

I don't know what variable would be null and why. Rewrite the logic to handle the else case here.

// We iterate over the tf.function's parameters positions.
for (int i = 0; i < callFunction.args.length; i++) {
// From the position i, we use the tf.function's definition to verify which parameter we are analyzing.
String evaluatedArgument = argumentIdDeclaringDefintion.get(i);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what happened to my comment here, but my question about why a for each loop is not being used here still pertains. Note that this is a question.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it was on outdated code. But, either way, it popped up here again.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't want to for-each over the values of the parsed tf.function arguments (callFunction.args: exprType[]) because those expressions are the values of the arguments since we are evaluating positional arguments. Additionally, we also don't want to for-each over the values of the tf.function definition (argumentIdDeclaringDefintion :ArrayList<String>) because there we have all keywords of tf.function arguments.

We want to see which arguments are being used for the parsed tf.function and match them with the position of the tf.function decorator argument, so we would need the index value. In this code, we get how many arguments the decorator we are parsing has and then tie those positions to the positions in the tf.function definition.

Copy link
Member

Choose a reason for hiding this comment

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

I see the problem now. You are indexing argumentIdDeclaringDefintion using callFunction.args.length as a length instead of argumentIdDeclaringDefintion.size(). But, nowhere do I see a comparison of callFunction.args.length with argumentIdDeclaringDefintion.size(). That's very dangerous if callFunction.args.length > argumentIdDeclaringDefintion.size().

Copy link
Member Author

Choose a reason for hiding this comment

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

For it to be valid Python code, you cannot have this case callFunction.args.length > argumentIdDeclaringDefintion.size(). You would receive TypeError: function() takes from 0 to 10 positional arguments but 11 were given (If 11 were given)

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a check for this.

/**
* Get the name of the module defining the entity described in the given {@link PySelection}.
* Get the set of potential declaring definitions of the entity described in the given {@link PySelection}.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I am still not convinced. To put it another way, why was it working before but now it is not working specifically for this feature?

* Test for #106. This tests whether we can parse one tf.function positional argument.
*/
@Test
public void testPositionalParameters() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

What test code?

Function function = functions.iterator().next();
assertNotNull(function);

Function.HybridizationParameters args = function.getHybridizationParameters();
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how this is resolved if it's blocked.

@@ -108,6 +142,10 @@ public HybridizationParameters(IProgressMonitor monitor) throws BadLocationExcep
// Will contain the last tf.function decorator
decoratorsType tfFunctionDecorator = null;

// Declaring definitions of the decorator, if it contains multiple definitions there might be more than one in this set. Since
// we are dealing with tf.function, we expect only one.
Copy link
Member

Choose a reason for hiding this comment

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

We can't "expect." We must check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I modified the comment.

if (potentialDeclaringDefitinion.iterator().hasNext())
declaringDefinition = potentialDeclaringDefitinion.iterator().next();
else
throw new IllegalStateException("Can't determine tf.function decorator defintion.");
Copy link
Member

@khatchad khatchad Mar 21, 2023

Choose a reason for hiding this comment

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

What is the point of changing existing code from returning a non scalar value to a scalar value and then only using one of them anyway?

Copy link
Member

Choose a reason for hiding this comment

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

Also, the error message here isn't very informative.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added more information about which param couldn't be determined.


// Matching the arguments from the definition and the arguments from the code being analyzed.
if (evaluatedArgument.equals(FUNC))
// Found parameter func
Copy link
Member

Choose a reason for hiding this comment

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

The comment here is not necessary. It's obvious what is being "found."

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed it.

// Found parameter experimental_follow_type_hints
this.experimentaFollowTypeHintsParamExists = true;
else
throw new IllegalArgumentException("Unable to process tf.function argument.");
Copy link
Member

Choose a reason for hiding this comment

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

Vague.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added more information.

@@ -164,11 +256,48 @@ else if (name.id.equals(EXPERIMENTAL_AUTOGRAPH_OPTIONS))
else if (name.id.equals(EXPERIMENTAL_FOLLOW_TYPE_HINTS))
// Found parameter experimental_follow_type_hints
this.experimentaFollowTypeHintsParamExists = true;
else
throw new IllegalArgumentException("Unable to process tf.function argument.");
Copy link
Member

Choose a reason for hiding this comment

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

In a PR for positional arguments, why are there changes to keyword arguments?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

* @param declaringDefinition The Definition to use.
* @return An array with the names of the arguments given by {@link Definition}.
*/
private ArrayList<String> getTfFunctionPythonDefinitionArguments(Definition declaringDefinition) {
Copy link
Member

Choose a reason for hiding this comment

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

This method doesn't seem very specific to tf.function at all. Isn't it more general?

Copy link
Member Author

Choose a reason for hiding this comment

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

I modified the name of the method and the comments so that it is more general. Thanks.

Copy link
Member

@khatchad khatchad left a comment

Choose a reason for hiding this comment

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

Many things are unclear and puzzling.

Copy link
Member

@khatchad khatchad left a comment

Choose a reason for hiding this comment

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

Potentially dangerous scalar indexing.

// We iterate over the tf.function's parameters positions.
for (int i = 0; i < callFunction.args.length; i++) {
// From the position i, we use the tf.function's definition to verify which parameter we are analyzing.
String evaluatedArgument = argumentIdDeclaringDefintion.get(i);
Copy link
Member

Choose a reason for hiding this comment

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

I see the problem now. You are indexing argumentIdDeclaringDefintion using callFunction.args.length as a length instead of argumentIdDeclaringDefintion.size(). But, nowhere do I see a comparison of callFunction.args.length with argumentIdDeclaringDefintion.size(). That's very dangerous if callFunction.args.length > argumentIdDeclaringDefintion.size().

@@ -143,7 +143,7 @@ public HybridizationParameters(IProgressMonitor monitor) throws BadLocationExcep
decoratorsType tfFunctionDecorator = null;

// Declaring definitions of the decorator, if it contains multiple definitions there might be more than one in this set. Since
// we are dealing with tf.function, we expect only one.
// we are dealing with tf.function, we check we it has one definition.
Copy link
Member

Choose a reason for hiding this comment

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

It's not only the comment. The checking must be in the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a check in the code.

@@ -164,61 +164,62 @@ public HybridizationParameters(IProgressMonitor monitor) throws BadLocationExcep
if (potentialDeclaringDefitinion.iterator().hasNext())
declaringDefinition = potentialDeclaringDefitinion.iterator().next();
else
throw new IllegalStateException("Can't determine tf.function decorator defintion.");
throw new IllegalStateException(String.format(
"Can't find declaring definition for selection: %s in line: %s, file: %s, and project: %s.",
Copy link
Member

Choose a reason for hiding this comment

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

This error message is vague and doesn't resemble the one that was modified.

}
} catch (AmbiguousDeclaringModuleException e) {
throw new IllegalStateException("Can't determine whether decorator: " + decorator + " is hybrid.", e);
}
} // We expect to have the last tf.function decorator in tfFunctionDecorator

// Getting tf.functions Python definition arguments.
ArrayList<String> argumentIdDeclaringDefintion = getTfFunctionPythonDefinitionArguments(declaringDefinition);
ArrayList<String> argumentIdDeclaringDefintion = getPythonDefinitionArguments(declaringDefinition);
Copy link
Member

Choose a reason for hiding this comment

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

What is a "Python definition argument?"

Copy link
Member Author

Choose a reason for hiding this comment

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

Getting the argument from the Python definition

@@ -256,20 +257,18 @@ else if (name.id.equals(EXPERIMENTAL_AUTOGRAPH_OPTIONS))
else if (name.id.equals(EXPERIMENTAL_FOLLOW_TYPE_HINTS))
// Found parameter experimental_follow_type_hints
this.experimentaFollowTypeHintsParamExists = true;
else
throw new IllegalArgumentException("Unable to process tf.function argument.");
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed? Now, we are missing the else case here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this is for keyword arguments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants