-
Notifications
You must be signed in to change notification settings - Fork 54
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
[Dynamo] Refactoring code for Hidet remote compilation #369
[Dynamo] Refactoring code for Hidet remote compilation #369
Conversation
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.
Can you follow the PR title format and add a description?
node: SymNode = example_input.node | ||
try: | ||
inputs.append(node.pytype(example_input)) | ||
except RuntimeError: |
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.
should we be concerned here?
RuntimeError seems to be quite generic so it might not just be the error that you think you can handle. I suggest we log a warning down below if it does not happen too often.
If possible, I prefer that you check the pre-condition that would cause the runtime error and instead of try-except, use
if not (the condition that would trigger runtime error):
inputs.append(...)
else:
# is a symbolic scalar input
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 see that this part is ported from the original code so you might not know why. Please ignore it for now
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.
Hi @xinli-git, FYI, we (Allan and I) have tried that, but find out it is hard to find the the condition that would trigger runtime error
.
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.
Thanks for the PR!
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.
It looks good to me! Thanks @destefy.
node: SymNode = example_input.node | ||
try: | ||
inputs.append(node.pytype(example_input)) | ||
except RuntimeError: |
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.
Hi @xinli-git, FYI, we (Allan and I) have tried that, but find out it is hard to find the the condition that would trigger runtime error
.
Closes #367 Also fixed an error mentioned [here](CentML/hidet#341 (comment)), which was encountered immediately after fixing the error described in #367
Closes #367 Also fixed an error mentioned [here](CentML/hidet#341 (comment)), which was encountered immediately after fixing the error described in #367
Closes #367 Also fixed an error mentioned [here](CentML/hidet#341 (comment)), which was encountered immediately after fixing the error described in #367
Hidet remote compilation needs to access the FlowGraph in order to hash it, so it has been moved into a separate function (get_flow_graph)
Remote compilation also needs to access the CompilationGraph since it will send that from the server to the client, so it as been moved into a separate function (get_compiled_graph)
Since the remote compilation client receives the CompilationGraph, the executor and wrapper function that uses this cgraph are moved to a separate function (get_wrapper)