Fix missing unistd.h on Windows #152

xsacha · 2020-07-20T03:11:14Z

Description

As part of Windows compatibility, a simple replacement for the two unistd functions is provided.
Also add in a missing export that is necessary on Windows.

Type of change

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas and hacks
I have made corresponding changes to the documentation and have regenerated the documentation (make html in docsrc)
I have added tests to verify my fix or my feature
New and existing unit tests pass locally with my changes

xsacha · 2020-07-20T05:19:09Z

Note that with this change I am able to successfully build TRTorch for Windows.
trtorch-0.0.3.zip

Built version shown here:
Linked against libtorch 1.6-rc3 + TensorRT 7.0.0.11 + CUDA 10.2 + cudnn7

narendasan · 2020-07-20T18:32:06Z

@xsacha Thanks for the contribution! We will try to verify the patch and get it merged in as soon as possible, though for the foreseeable future windows support is going to remain unofficial / mostly community driven due to developer resource constraints. We are more than happy to merge / review PRs and help where we can but it will not be a high priority for us. Also in order to merge we need you to sign your commit in accordance with the Developer Certificate of Origin (https://github.com/NVIDIA/TRTorch/blob/master/CONTRIBUTING.md#commits-and-prs). To amend your commits follow these instructions: https://github.com/NVIDIA/TRTorch/pull/152/checks?check_run_id=888297024

As part of Windows compatibility Signed-off-by: Sacha Refshauge <[email protected]>

Signed-off-by: Sacha Refshauge <[email protected]>

narendasan · 2020-07-21T21:14:55Z

@xsacha Were there associated build system changes you made to get the library to compile?

xsacha · 2020-07-21T21:27:51Z

I needed to change the bazel files to point to Windows dlls but I am not familiar with the proper way to do this so I didn't commit these. I had just changed the default to Windows.

narendasan · 2020-07-21T21:32:55Z

Ideally we could use bazel platforms like we do for aarch64-linux (e.g. https://github.com/NVIDIA/TRTorch/blob/fe06d0993d1ebf9083eada67a42e4fd46c4bee92/third_party/tensorrt/local/BUILD#L3, documentation: https://docs.bazel.build/versions/master/platforms-intro.html) if you want to take a stab at it or you could commit the changes you made and we can advise on next steps.

xsacha · 2020-07-22T03:54:03Z

Do you want the Bazel changes in this PR as well?

Note: you will need to use local instead of archive for cuda/cudnn/tensorrt. Also: I was not able to get git working in Bazel but it is meant to work. If it doesn't just comment that out and load the pip repository manually. Signed-off-by: Sacha Refshauge <[email protected]>

narendasan · 2020-07-22T16:14:03Z

Yeah having them in this PR is fine

andi4191

I have a few queries and requests

cpp/api/include/trtorch/trtorch.h

cpp/trtorchc/main.cpp

third_party/libtorch/BUILD

Signed-off-by: Sacha Refshauge <[email protected]>

andi4191

LGTM

narendasan

Noticed a couple syntax issues and also when I tried compiling I ran into some errors, seems like the have to do with the logging macros. Did you run into these?

core/conversion/conversion.cpp(60): error C2679: binary '<<': no operator found which takes a right-hand operand of type 'trtorch::core::util::logging::TRTorchLogger' (or there is no acceptable conversion)

Also just curious when you do development on Windows do you typically install libraries like cudnn or tensorrt to the system somewhere and reuse them or do you keep them in a workspace that is project specific. Seems like we could extend these changes to the archive based dependencies too pretty easily. Doesn't have to be in this PR but just wondering if it would be useful.

narendasan · 2020-07-23T05:54:59Z

third_party/cudnn/local/BUILD

@@ -19,6 +26,7 @@ cc_import(
    name = "cudnn_lib",
    shared_library = select({
        ":aarch64_linux": "lib/aarch64-linux-gnu/libcudnn.so",
+        ":windows": glob(["bin/cudnn64_*.dll"])[0],


This is kind of odd, is there a reason you chose glob here?

Just to note, the glob failed, I ended up just putting the 7 in the path and removing the glob

How do we support cudnn8?
I was trying to make sure it supports v8 and the glob worked for me to pick 7 or 8.

I'm not good with bazel and couldn't figure out how to do a wildcard without a glob. Then a glob returns a list so I needed to get the first value.

Also just curious when you do development on Windows do you typically install libraries like cudnn or tensorrt to the system somewhere and reuse them or do you keep them in a workspace that is project specific. Seems like we could extend these changes to the archive based dependencies too pretty easily. Doesn't have to be in this PR but just wondering if it would be useful.

I installed cudnn and tensorrt to a folder and specified it in WORKSPACE. I'm more of a Linux developer but need this working on Windows if we are to use it at all.

Not sure what's going on with the stream operator there. From the error, it looks like it doesn't like:

#define TRTORCH_LOG(l, sev, msg) \ do { \ std::stringstream ss{}; \ ss << msg; \ l.log(sev, ss.str()); \ } while (0)

Maybe a MSVC issue (if that's the compiler you are using).
If you remove the line:
ss << msg;
does it compile?

I can look into if there is another way to get the wildcard working. Hmm, I can look into the logging issue more, you didn't hit it right? What version of MSVC did you use? Did you add any additional flags or anything (like I noticed MSVC doesnt handle default flags we have like --std=c++14)?

@narendasan Any update with this?
It seems to work fine here. Is it the version of Bazel I am using?

Seems like something about the particular use of a specialized logger in conversion.cpp that is causing the issue, If I switch to the generic logger I can compile the library and successfully start programs using the core library such as trtorchc (I don't notice the issue mentioned in #153 if i understood the issue correctly either, the compiler starts up correctly but I think we don't handle windows paths well so i haven't yet tested further). I dont think it is due to the bazel version as it just calls the underlying C++ compiler similar to how make or CMake does. I think its a compiler issue, I am not sure which compiler version you used, I used the latest MSVC 2019 build tools. If there is anymore information you could give me about your build environment and build process that would be helpful. I think other than that issue this PR is good to merge though.

I mean for the globbing issue. Is this related to the bazel version?

No I think it may have been a path error on my part i think. It works now.

third_party/tensorrt/local/BUILD

Signed-off-by: Sacha Refshauge <[email protected]>

narendasan · 2020-07-29T06:32:58Z

Solved the issue with the macros in conversion.cpp. Can you add this in place of the top level log macros. https://github.com/NVIDIA/TRTorch/blob/f8bea3bf07493a78501b6c098da7615ea584bd18/core/util/macros.h#L28

#ifdef _MSC_VER

#define EXPAND( x ) x

#define LOG_GRAPH(...)          EXPAND(GET_MACRO(__VA_ARGS__, LOG_GRAPH_OWN, LOG_GRAPH_GLOBAL)(__VA_ARGS__))
#define LOG_DEBUG(...)          EXPAND(GET_MACRO(__VA_ARGS__, LOG_DEBUG_OWN, LOG_DEBUG_GLOBAL)(__VA_ARGS__))
#define LOG_INFO(...)           EXPAND(GET_MACRO(__VA_ARGS__, LOG_INFO_OWN, LOG_INFO_GLOBAL)(__VA_ARGS__))
#define LOG_WARNING(...)        EXPAND(GET_MACRO(__VA_ARGS__, LOG_WARNING_OWN, LOG_WARNING_GLOBAL)(__VA_ARGS__))
#define LOG_ERROR(...)          EXPAND(GET_MACRO(__VA_ARGS__, LOG_ERROR_OWN, LOG_ERROR_GLOBAL)(__VA_ARGS__))
#define LOG_INTERNAL_ERROR(...) EXPAND(GET_MACRO(__VA_ARGS__, LOG_INTERNAL_ERROR_OWN, LOG_INTERNAL_ERROR_GLOBAL)(__VA_ARGS__))

#else

#define LOG_GRAPH(...)          GET_MACRO(__VA_ARGS__, LOG_GRAPH_OWN, LOG_GRAPH_GLOBAL)(__VA_ARGS__)
#define LOG_DEBUG(...)          GET_MACRO(__VA_ARGS__, LOG_DEBUG_OWN, LOG_DEBUG_GLOBAL)(__VA_ARGS__)
#define LOG_INFO(...)           GET_MACRO(__VA_ARGS__, LOG_INFO_OWN, LOG_INFO_GLOBAL)(__VA_ARGS__)
#define LOG_WARNING(...)        GET_MACRO(__VA_ARGS__, LOG_WARNING_OWN, LOG_WARNING_GLOBAL)(__VA_ARGS__)
#define LOG_ERROR(...)          GET_MACRO(__VA_ARGS__, LOG_ERROR_OWN, LOG_ERROR_GLOBAL)(__VA_ARGS__)
#define LOG_INTERNAL_ERROR(...) GET_MACRO(__VA_ARGS__, LOG_INTERNAL_ERROR_OWN, LOG_INTERNAL_ERROR_GLOBAL)(__VA_ARGS__)

#endif

Signed-off-by: Sacha Refshauge <[email protected]>

narendasan

Awesome looks like everything is compiling, Should be good to merge! Thanks for sticking with it.

xsacha mentioned this pull request Jul 20, 2020

🐛 [Bug] Windows crash at runtime #153

Closed

xsacha and others added 2 commits July 21, 2020 08:59

Update main.cpp

53c3430

As part of Windows compatibility Signed-off-by: Sacha Refshauge <[email protected]>

Include missing export on ExtraInfo::DataType

1a00287

Signed-off-by: Sacha Refshauge <[email protected]>

xsacha force-pushed the patch-2 branch from 58f9a03 to 1a00287 Compare July 20, 2020 23:02

xsacha force-pushed the patch-2 branch from 5da8edb to c98832d Compare July 22, 2020 04:23

narendasan requested review from andi4191 and narendasan July 22, 2020 16:13

andi4191 suggested changes Jul 23, 2020

View reviewed changes

cpp/api/include/trtorch/trtorch.h Show resolved Hide resolved

cpp/trtorchc/main.cpp Outdated Show resolved Hide resolved

cpp/trtorchc/main.cpp Outdated Show resolved Hide resolved

third_party/libtorch/BUILD Show resolved Hide resolved

Changes as requested: #if/#else

6150abe

Signed-off-by: Sacha Refshauge <[email protected]>

andi4191 approved these changes Jul 23, 2020

View reviewed changes

narendasan requested changes Jul 23, 2020

View reviewed changes

Missing commas in tensorrt bazel

4ade7eb

Signed-off-by: Sacha Refshauge <[email protected]>

Fix logging on MSVC

6346b1c

Signed-off-by: Sacha Refshauge <[email protected]>

narendasan self-requested a review July 29, 2020 21:48

narendasan approved these changes Jul 29, 2020

View reviewed changes

narendasan merged commit 703947f into pytorch:master Jul 29, 2020

Fix missing unistd.h on Windows #152

Fix missing unistd.h on Windows #152

Uh oh!

Conversation

xsacha commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist:

Uh oh!

xsacha commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

narendasan commented Jul 20, 2020

Uh oh!

narendasan commented Jul 21, 2020

Uh oh!

xsacha commented Jul 21, 2020

Uh oh!

narendasan commented Jul 21, 2020

Uh oh!

xsacha commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

narendasan commented Jul 22, 2020

Uh oh!

andi4191 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andi4191 left a comment

Choose a reason for hiding this comment

Uh oh!

narendasan left a comment

Choose a reason for hiding this comment

Uh oh!

narendasan Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

narendasan Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xsacha Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

xsacha Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

narendasan Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

xsacha Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

narendasan Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

xsacha Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

narendasan Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

narendasan commented Jul 29, 2020

Uh oh!

narendasan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xsacha commented Jul 20, 2020 •

edited

Loading

xsacha commented Jul 20, 2020 •

edited

Loading

xsacha commented Jul 22, 2020 •

edited

Loading

narendasan Jul 23, 2020 •

edited

Loading

xsacha Jul 23, 2020 •

edited

Loading