Merged
Conversation
Lafi7e
added
training
mrry
reviewed
Jun 11, 2021
Comment on lines
+232
to
+236
| auto tensor_type = DataTypeImpl::GetType<Tensor>(); | ||
| std::function<void(void*)> deleter = [dlpack, tensor_type](void* p) { | ||
| dlpack->deleter((dlpack)); | ||
| tensor_type->GetDeleteFunc()(p); | ||
| }; |
Contributor
There was a problem hiding this comment.
Does this slightly simpler code work? It probably doesn't make a big difference, but it seems a pity to allocate space in the closure for a value that is always the same.
Suggested change
| auto tensor_type = DataTypeImpl::GetType<Tensor>(); | |
| std::function<void(void*)> deleter = [dlpack, tensor_type](void* p) { | |
| dlpack->deleter((dlpack)); | |
| tensor_type->GetDeleteFunc()(p); | |
| }; | |
| std::function<void(void*)> deleter = [dlpack](void* p) { | |
| dlpack->deleter(dlpack); | |
| DataTypeImpl::GetType<Tensor>()->GetDeleteFunc()(p); | |
| }; |
(Even better would be to make tensor_type into a static local, but I'm not sure if that's legal.)
pengwa
previously approved these changes
Jun 11, 2021
pengwa
reviewed
Jun 11, 2021
ytaous
approved these changes
Jun 11, 2021
Contributor
ytaous
left a comment
ytaous
left a comment
There was a problem hiding this comment.
Checked again with valgrind logs, still clean. Will check e2e again after merge.
harshithapv
pushed a commit
that referenced
this pull request
Jun 16, 2021
harshithapv
added a commit
that referenced
this pull request
Jun 18, 2021
* Cache initializers and avoid device check ot end of forward (#7905) * ATenOp Enhancement (#7725) * config parser, default argument values * ut * win build * maxpool2d * fix win build * fix build * unfold atenop * Update CMakeLists.txt for openvino EP (#7980) * Add SoftmaxCrossEntropyLossInternal to Support Dynamic ignore_index Input (#7899) * add SoftmaxCrossEntropyLossInternal * bugfix and ut * fix ut * fix ut * support torch1.8.1 * function body for nll_loss_internal * Override ORTModule named_modules to support extra arg (#7954) * add missing provider_options.h in packages (#7995) * consolidate copy binary script for gpu/trt tarball package * add provider_options.h * add provider_options.h * Add cuda provides files (#8002) * Save module output for backward if needed (#8010) * Save module output for backward if needed * Make logic in InsertCastTransformer around forcing a node to fp32 more precise. (#8018) * Address #7981 Reworked the logic around forcing a node to run on fp32 even if it was supported on fp16. The github issue had multiple factors. In ORT 1.8 we remove Identity nodes that produce graph outputs as they're not needed. That resulted in a Loop node no longer having output nodes (it produces graph outputs instead), which meant the check in IsSingleInputNodeFloat16Node returned true as there was no longer a downstream Identity node processing fp16 data. We shouldn't only force a node to fp32 in very specific circumstances, and the changes hopefully check for those more precisely. * Fix Memory Leak from DlpackToOrtValue (#8029) * Update DirectML EP changes from DmlDev as of 2021-06-07 (#7987) * Merged PR 6093117: Fix test_DynamicQuantizedLinear_max_adjusted_expanded by allowing Identity operator to run on non-float inputs Motivation: As part of the OnnxConformance Backend tests, DynamicQuantizedLinear_max_adjusted_expanded is failing. Root Cause: - The test model has `Identity` operator as one of the node. The input of this node is of non-float data type. - In DML, `Identity` operator is registered as operator which requires floating input. - As per `DirectMLSchema.h`, support for non-float input has been added for `Identity` operator in DML but the same has not been reflected in the `OperatorRegistration.cpp`. Changes: - Removed all traces of the requiresFloatFormatsForGraph flag from it's definition and usage. This flag was only used for Identity and it's related operator. - Added null check for the graphOutput nodeArg in GraphDescBuilder.cpp to stop the crash of the test. Related work items: #33076298 * Merged PR 6103324: Remove usage of non-generic error code (FWP_E_NULL_POINTER) Motivation: Addressing Dwayne comment on the previous PR. [Ref: [6093117](https://dev.azure.com/microsoft/WindowsAI/_git/onnxruntime/pullrequest/6093117?discussionId=44292162&path=%2Fonnxruntime%2Fcore%2Fproviders%2Fdml%2FDmlExecutionProvider%2Fsrc%2FGraphPartitioner.cpp)] Changes: Inside the DML EP, we should not use some other platform specific error codes. Instead we should a appropriate generic error code. Related work items: #33076298 Co-authored-by: Sumit Agarwal <sumitagarwal@microsoft.com> * [js/react_native] Use a mobile ORT instead of a full ORT (#8042) * Change full ort to mobile ort * Update Android example to load mobile ort * Change the format of test models to ort * update ios to use mobile ort * revise README * use onnxruntime-mobile-c CocoaPods in a npm package * fix PATH addition in windows should set PATH, not add to the tail the copy of PATH * Reduce Kernel Optimization (#8067) * reduce optimization * bug fix * add a check * add ut * refactor * add ut cases for keepdims=true Co-authored-by: baijumeswani <bmeswani@microsoft.com> Co-authored-by: Vincent Wang <wangwchpku@outlook.com> Co-authored-by: Changming Sun <chasun@microsoft.com> Co-authored-by: George Wu <jywu@microsoft.com> Co-authored-by: Ryan Hill <38674843+RyanUnderhill@users.noreply.github.com> Co-authored-by: Sherlock <baihan.huang@gmail.com> Co-authored-by: Scott McKay <skottmckay@gmail.com> Co-authored-by: sumitsays <sumitagarwal330@gmail.com> Co-authored-by: Sumit Agarwal <sumitagarwal@microsoft.com> Co-authored-by: Sunghoon <35605090+hanbitmyths@users.noreply.github.com> Co-authored-by: iperov <lepersorium@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix Memory Leak from DlpackToOrtValue. Currently the deleter in DlpackToOrtValue deletes the torch tensor (the data), but fails to delete the Tensor object created in ORT by the make_unique.