-
Notifications
You must be signed in to change notification settings - Fork 16
add Extract method to feature-extractor server #188
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
Conversation
|
Please let me know if you think I should drop per-extractor methods. It would simplify the code. |
| if request.HasField(name): | ||
| options = getattr(request, name, None) | ||
| constructor = getattr(self, "_%s_extractor" % name) | ||
| extractors.append(constructor(options)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor functions do not check for options to be None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I understand generated grpc code it can't be None if request.HasField returns true. But I added just check just in case something changes. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok, didn't know that! But yup, better be sure by still adding the check 👍
src/main/proto/service.proto
Outdated
| } | ||
|
|
||
| message GraphletOptions { | ||
| int32 docfreqThreshold = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's ok to skip 1 and keep the same order as when this was in GraphletRequest, but is there a reason to do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, no reason. I missed it because to copy-past. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Currently, we call feature-extractor 3 times per file in file mode and number of functions * number of files * 3 in function mode. Each time we send full uast to feature-extractor server. This commit introduces new Extract method that can run different extractors sequentially on UAST and return features from all of them. I kept all previous methods both in server and client but changed request parameters so Extract and old method could share extractors configuration. Signed-off-by: Maxim Sukharev <[email protected]>
|
Sorry for push-force but changes required code regeneration and it became a mess anyway. Fixes applied. I'll merge after CI is green then. |
Currently, we call feature-extractor 3 times per file in file mode and
number of functions * number of files * 3in function mode.Each time we send full uast to feature-extractor server.
This commit introduces new Extract method that can run different
extractors sequentially on UAST and return features from all of them.
I kept all previous methods both in server and client but changed
request parameters so Extract and old method could share extractors
configuration.