Skip to content

CR feedback #2970

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.

Sign up for GitHub

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

Jump to bottom
Merged
smk2007 merged 6 commits into from
Feb 4, 2020
Merged

CR feedback #2970

smk2007 merged 6 commits into from
Feb 4, 2020

Conversation

@smk2007
Copy link
Member

@smk2007 smk2007 commented Feb 3, 2020

User/sheilk/windowsai merge cr feedback

@smk2007 smk2007 requested a review from a team as a code owner February 3, 2020 23:50
ryanlai2
ryanlai2 reviewed Feb 3, 2020
View reviewed changes
fdwr
fdwr reviewed Feb 3, 2020
View reviewed changes
include/onnxruntime/core/session/onnxruntime_c_api.h
* This api augments OrtTypeInfo to return denotations on the type.
* This is used by WinML to determine if an input/output is intended to be an Image or a Tensor.
*/
OrtStatus*(ORT_API_CALL* GetDenotationFromTypeInfo)(_In_ const OrtTypeInfo*, _Out_ const char** const denotation, _Out_ size_t* len)NO_EXCEPTION;
Copy link
Contributor

@fdwr fdwr Feb 3, 2020 •
edited by smk2007
Loading

Choose a reason for hiding this comment

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

[nit] Add some spaces for readability before and after the function? Or is stupid clang-format messing things up?

OrtStatus* (...)(...) NO_EXCEPTION; #Resolved

Copy link
Contributor

@fdwr fdwr Feb 3, 2020 •
edited by smk2007
Loading

Choose a reason for hiding this comment

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

Ah, seems you just cut&paste this from elsewhere. Non-blocking minor. #Resolved

Copy link
Member Author

@smk2007 smk2007 Feb 3, 2020

Choose a reason for hiding this comment

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

same formatting as other places in this file!

In reply to: 374407768 [](ancestors = 374407768)

Copy link
Member Author

@smk2007 smk2007 Feb 3, 2020

Choose a reason for hiding this comment

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

yep

In reply to: 374408058 [](ancestors = 374408058)

snnn
snnn approved these changes Feb 3, 2020
View reviewed changes
Copy link
Contributor

@snnn snnn left a comment

Choose a reason for hiding this comment

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

Thanks

tiagoshibata
tiagoshibata reviewed Feb 3, 2020
View reviewed changes
@smk2007
Copy link
Member Author

smk2007 commented Feb 3, 2020

YW!

In reply to: 352644894 [](ancestors = 352644894)

ryanlai2
ryanlai2 reviewed Feb 4, 2020
View reviewed changes
cmake/precompiled_header.cmake
@@ -1,3 +1,6 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Contributor

@ryanlai2 ryanlai2 Feb 4, 2020 •
edited by smk2007
Loading

Choose a reason for hiding this comment

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

All rights reserved. [](start = 39, length = 20)

Why keep "All rights reserved" here and remove it for the source files? #Resolved

Copy link
Member Author

@smk2007 smk2007 Feb 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh copy/paste error.

In reply to: 374411856 [](ancestors = 374411856)

Copy link
Contributor

@ryanlai2 ryanlai2 Feb 4, 2020

Choose a reason for hiding this comment

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

Same thing for the other .cmake files and also in CMakeLists.txt?

In reply to: 374412145 [](ancestors = 374412145,374411856)

Copy link
Member Author

@smk2007 smk2007 Feb 4, 2020

Choose a reason for hiding this comment

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

hmm... seems like All rights reserved is more common in the code base... switching to that.

In reply to: 374412543 [](ancestors = 374412543,374412145,374411856)

zhangxiang1993
zhangxiang1993 reviewed Feb 4, 2020
View reviewed changes
winml/adapter/winml_adapter_dml.cpp
@@ -1,5 +1,6 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Contributor

@zhangxiang1993 zhangxiang1993 Feb 4, 2020 •
edited by smk2007
Loading

Choose a reason for hiding this comment

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

revert? #Resolved

zhangxiang1993
zhangxiang1993 reviewed Feb 4, 2020
View reviewed changes
winml/adapter/winml_adapter_environment.cpp
@@ -1,5 +1,6 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Contributor

@zhangxiang1993 zhangxiang1993 Feb 4, 2020 •
edited by smk2007
Loading

Choose a reason for hiding this comment

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

revert?
#Resolved

zhangxiang1993
zhangxiang1993 reviewed Feb 4, 2020
View reviewed changes
winml/adapter/winml_adapter_execution_provider.cpp
@@ -1,5 +1,6 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Contributor

@zhangxiang1993 zhangxiang1993 Feb 4, 2020 •
edited by smk2007
Loading

Choose a reason for hiding this comment

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

same. #Resolved

zhangxiang1993
zhangxiang1993 reviewed Feb 4, 2020
View reviewed changes
cmake/precompiled_header.cmake Outdated
@@ -1,3 +1,6 @@
# Copyright (c) Microsoft Corporation.
Copy link
Contributor

@zhangxiang1993 zhangxiang1993 Feb 4, 2020 •
edited by smk2007
Loading

Choose a reason for hiding this comment

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

All rights reserved. #Resolved

@zhangxiang1993
Copy link
Contributor

zhangxiang1993 commented Feb 4, 2020 •
edited
Loading 

 python $(Build.SourcesDirectory)\tools\ci_build\build.py --config $(BuildConfig) --build_dir $(Build.BinariesDirectory) --skip_submodule_sync --build_shared_lib --test --cmake_generator "Visual Studio 16 2019" --msvc_toolset 14.16 --build_wheel --use_featurizers --use_dnnl --build_shared_lib --enable_onnx_tests --use_dml --use_cuda --cuda_version=10.0 --cuda_home="C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v10.0" --cudnn_home="C:\local\cudnn-10.0-windows10-x64-v7.6.5.32\cuda" --cmake_extra_defines CMAKE_SYSTEM_VERSION=10.0.18362.0

nit: two lines #Resolved

Refers to: tools/ci_build/github/azure-pipelines/win-gpu-ci-pipeline.yml:119 in 784cefa. [](commit_id = 784cefa, deletion_comment = False)

zhangxiang1993
zhangxiang1993 approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@zhangxiang1993 zhangxiang1993 left a comment

Choose a reason for hiding this comment

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

:shipit:

@smk2007 smk2007 merged commit 3f0b635 into windowsai Feb 4, 2020
@smk2007 smk2007 deleted the user/sheilk/windowsai-merge-cr-feedback branch February 4, 2020 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fdwr fdwr fdwr left review comments

@walrusmcd walrusmcd Awaiting requested review from walrusmcd

@orilevari orilevari Awaiting requested review from orilevari

@martinb35 martinb35 Awaiting requested review from martinb35

+4 more reviewers

@tiagoshibata tiagoshibata tiagoshibata left review comments

@ryanlai2 ryanlai2 ryanlai2 left review comments

@snnn snnn snnn approved these changes

@zhangxiang1993 zhangxiang1993 zhangxiang1993 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@smk2007 @zhangxiang1993 @snnn @fdwr @tiagoshibata @ryanlai2