port class 'process' to Windows #2277

apwojcik · 2023-10-03T21:23:16Z

No description provided.

codecov · 2023-10-03T21:31:20Z

Codecov Report

Merging #2277 (ae60946) into develop (e5cd8b6) will increase coverage by 0.00%.
Report is 1 commits behind head on develop.
The diff coverage is 100.00%.

❗ Current head ae60946 differs from pull request most recent head c40ffad. Consider uploading reports for the commit c40ffad to get more accurate results

@@           Coverage Diff            @@
##           develop    #2277   +/-   ##
========================================
  Coverage    91.45%   91.45%           
========================================
  Files          433      433           
  Lines        16173    16179    +6     
========================================
+ Hits         14791    14797    +6     
  Misses        1382     1382

Files	Coverage Δ
src/process.cpp	`73.33% <100.00%> (+1.24%)`	⬆️

... and 4 files with indirect coverage changes

pfultz2 · 2023-10-03T22:27:17Z

src/CMakeLists.txt

@@ -104,6 +103,11 @@ add_library(migraphx
    value.cpp
    verify_args.cpp
 )
+if(WIN32)
+    target_sources(migraphx PRIVATE process_win32.cpp)


Dont create another source file, just add an ifdef to the process.cpp file.

Ok, I will move it to the process.cpp. I thought you asked me to do that in the Draft PR, but I might mistaken that with someone else asking me from a different component.

pfultz2 · 2023-10-03T22:27:45Z

src/process_win32.cpp

+
+MIGRAPHX_DECLARE_ENV_VAR(MIGRAPHX_TRACE_CMD_EXECUTE)
+
+#define MIGRAPHX_PROCESS_BUFSIZE 4096


Use a const variable.

pfultz2 · 2023-10-03T22:39:32Z

Why arent you using _popen function, which is the equivalent for windows? I would assume we could just write something like this:

#ifdef WIN32
#define MIGRAPHX_PCLOSE _pclose
#define MIGRAPHX_POPEN _popen
#else
#define MIGRAPHX_PCLOSE pclose
#define MIGRAPHX_POPEN popen
#endif

template <class F>
int exec(const std::string& cmd, const char* type, F f)
{
    int ec = 0;
    if(enabled(MIGRAPHX_TRACE_CMD_EXECUTE{}))
        std::cout << cmd << std::endl;
    auto closer = [&](FILE* stream) {
        auto status = MIGRAPHX_PCLOSE(stream);
#ifdef WIN32
        ec = status < 0 : -1 : 0;
#else
        ec = WIFEXITED(status) ? WEXITSTATUS(status) : 0; // NOLINT
#endif
    };
    {
        // TODO: Use execve instead of popen
        std::unique_ptr<FILE, decltype(closer)> pipe(MIGRAPHX_POPEN(cmd.c_str(), type), closer); // NOLINT
        if(not pipe)
            MIGRAPHX_THROW("popen() failed: " + cmd);
        f(pipe.get());
    }
    return ec;
}

pfultz2 · 2023-10-03T22:41:05Z

src/process_win32.cpp

+        if(bSuccess == FALSE)
+            return GetLastError();
+
+        DWORD dwRead, dwWritten;


You need to run clang tidy on this. These variables do not match the naming scheme we use.

I will. I thought I did. This is Hungarian notation, the WIN32 API notation, and the most seen when someone is writing WIN32 API applications.

pfultz2 · 2023-10-03T22:42:50Z

src/process_win32.cpp

+
+void process::exec() { impl->check_exec(impl->get_command()); }
+
+void process::write(std::function<void(process::writer)> pipe_in)


The pipe_in variable is not used.

src/process_win32.cpp

pfultz2 · 2023-10-03T22:57:00Z

src/process_win32.cpp

+        if(SetHandleInformation(&hRead_, HANDLE_FLAG_INHERIT, 0) == FALSE)
+            throw GetLastError();
+    }
+


Need to add a move constructor, delete dopy constructor and add an assignment operator.

apwojcik · 2023-10-03T23:04:27Z

The MSDN documentation for _popen states:
If used in a Windows program, the _popen function returns an invalid file pointer that causes the program to stop responding indefinitely. _popen works properly in a console application. To create a Windows application that redirects input and output, see Creating a child process with redirected input and output in the Windows SDK.

apwojcik · 2023-10-03T23:07:50Z

More _popen creates a pipe and requires translation from POSIX to WIN32 and vice versa. That is a performance penalty we want to avoid, considering the UAI project requirements.

migraphx-bot · 2023-10-04T05:46:36Z

Test	Batch	Rate new ff1dbd	Rate old a3cf99	Diff	Compare
torchvision-resnet50	64	2,320.53	2,322.40	-0.08%	✅
torchvision-resnet50_fp16	64	5,353.32	5,360.94	-0.14%	✅
torchvision-densenet121	32	1,848.50	1,846.99	0.08%	✅
torchvision-densenet121_fp16	32	3,415.52	3,410.65	0.14%	✅
torchvision-inceptionv3	32	1,292.55	1,295.24	-0.21%	✅
torchvision-inceptionv3_fp16	32	2,542.53	2,536.02	0.26%	✅
cadene-inceptionv4	16	620.42	619.80	0.10%	✅
cadene-resnext64x4	16	588.75	588.66	0.02%	✅
slim-mobilenet	64	7,216.17	7,207.48	0.12%	✅
slim-nasnetalarge	64	236.47	236.47	0.00%	✅
slim-resnet50v2	64	2,557.30	2,557.75	-0.02%	✅
bert-mrpc-onnx	8	824.74	824.68	0.01%	✅
bert-mrpc-tf	1	388.52	388.60	-0.02%	✅
pytorch-examples-wlang-gru	1	299.62	300.03	-0.14%	✅
pytorch-examples-wlang-lstm	1	313.95	317.68	-1.17%	✅
torchvision-resnet50_1	1	547.35	551.05	-0.67%	✅
torchvision-inceptionv3_1	1	304.48	303.30	0.39%	✅
cadene-dpn92_1	1	354.27	356.34	-0.58%	✅
cadene-resnext101_1	1	219.75	218.29	0.67%	✅
slim-vgg16_1	1	224.23	223.99	0.11%	✅
slim-mobilenet_1	1	1,493.88	1,506.95	-0.87%	✅
slim-inceptionv4_1	1	215.56	214.27	0.60%	✅
onnx-taau-downsample	1	306.17	306.46	-0.10%	✅
dlrm-criteoterabyte	1	21.67	21.68	-0.02%	✅
dlrm-criteoterabyte_fp16	1	40.74	40.75	-0.03%	✅
agentmodel	1	5,820.82	5,812.37	0.15%	✅
unet_fp16	2	55.16	55.12	0.08%	✅
resnet50v1_fp16	1	754.23	757.48	-0.43%	✅
bert_base_cased_fp16	64	970.92	970.36	0.06%	✅
bert_large_uncased_fp16	32	305.14	304.99	0.05%	✅
bert_large_fp16	1	167.01	167.50	-0.29%	✅
distilgpt2_fp16	16	1,352.29	1,351.60	0.05%	✅

This build is OK for merge ✅

migraphx-bot · 2023-10-04T05:46:38Z

:white_check_mark:bert-mrpc-onnx: PASSED: MIGraphX meets tolerance

:white_check_mark:bert-mrpc-tf: PASSED: MIGraphX meets tolerance

:white_check_mark:pytorch-examples-wlang-gru: PASSED: MIGraphX meets tolerance

:white_check_mark:pytorch-examples-wlang-lstm: PASSED: MIGraphX meets tolerance

:white_check_mark:torchvision-resnet50_1: PASSED: MIGraphX meets tolerance

:white_check_mark:torchvision-inceptionv3_1: PASSED: MIGraphX meets tolerance

:white_check_mark:cadene-dpn92_1: PASSED: MIGraphX meets tolerance

:white_check_mark:cadene-resnext101_1: PASSED: MIGraphX meets tolerance

:white_check_mark:slim-vgg16_1: PASSED: MIGraphX meets tolerance

:white_check_mark:slim-mobilenet_1: PASSED: MIGraphX meets tolerance

:white_check_mark:slim-inceptionv4_1: PASSED: MIGraphX meets tolerance

:white_check_mark:dlrm-criteoterabyte: PASSED: MIGraphX meets tolerance

:white_check_mark:agentmodel: PASSED: MIGraphX meets tolerance

:white_check_mark:unet: PASSED: MIGraphX meets tolerance

:white_check_mark:resnet50v1: PASSED: MIGraphX meets tolerance

🔴bert_base_cased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output

🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output

:white_check_mark:bert_large: PASSED: MIGraphX meets tolerance

🔴distilgpt2_fp16: FAILED: MIGraphX is not within tolerance - check verbose output

pfultz2 · 2023-10-04T15:14:42Z

If used in a Windows program, the _popen function returns an invalid file pointer that causes the program to stop responding indefinitely. _popen works properly in a console application.

I see, it would be better to use tiny-process-library for this. The popen has limitations on POSIX side as well(ie no bidirectional pipes). This probably doesnt need to be switched in this PR, though.

port 'process' class to Windows

0b3129f

apwojcik added the Windows Related changes for Windows Environments label Oct 3, 2023

apwojcik requested a review from pfultz2 October 3, 2023 21:23

pfultz2 reviewed Oct 3, 2023

View reviewed changes

src/process_win32.cpp Outdated Show resolved Hide resolved

pfultz2 reviewed Oct 3, 2023

View reviewed changes

apwojcik added 2 commits October 8, 2023 11:02

Merge branch 'develop' into port_process_windows

f457d45

incorporate review feedback

e834074

pfultz2 approved these changes Oct 8, 2023

View reviewed changes

apwojcik and others added 2 commits October 8, 2023 20:46

fix cppcheck and format

ff1dbd7

Merge branch 'develop' into port_process_windows

c40ffad

causten merged commit adfc74a into develop Oct 10, 2023
14 of 15 checks passed

causten deleted the port_process_windows branch October 10, 2023 22:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port class 'process' to Windows #2277

port class 'process' to Windows #2277

apwojcik commented Oct 3, 2023

codecov bot commented Oct 3, 2023 •

edited

Loading

pfultz2 Oct 3, 2023

apwojcik Oct 3, 2023

pfultz2 Oct 3, 2023

pfultz2 commented Oct 3, 2023

pfultz2 Oct 3, 2023

apwojcik Oct 3, 2023

pfultz2 Oct 3, 2023

pfultz2 Oct 3, 2023

apwojcik commented Oct 3, 2023 •

edited

Loading

apwojcik commented Oct 3, 2023

migraphx-bot commented Oct 4, 2023 •

edited

Loading

migraphx-bot commented Oct 4, 2023

pfultz2 commented Oct 4, 2023


		MIGRAPHX_DECLARE_ENV_VAR(MIGRAPHX_TRACE_CMD_EXECUTE)

		#define MIGRAPHX_PROCESS_BUFSIZE 4096


		void process::exec() { impl->check_exec(impl->get_command()); }

		void process::write(std::function<void(process::writer)> pipe_in)

port class 'process' to Windows #2277

port class 'process' to Windows #2277

Conversation

apwojcik commented Oct 3, 2023

codecov bot commented Oct 3, 2023 • edited Loading

Codecov Report

pfultz2 Oct 3, 2023

Choose a reason for hiding this comment

apwojcik Oct 3, 2023

Choose a reason for hiding this comment

pfultz2 Oct 3, 2023

Choose a reason for hiding this comment

pfultz2 commented Oct 3, 2023

pfultz2 Oct 3, 2023

Choose a reason for hiding this comment

apwojcik Oct 3, 2023

Choose a reason for hiding this comment

pfultz2 Oct 3, 2023

Choose a reason for hiding this comment

pfultz2 Oct 3, 2023

Choose a reason for hiding this comment

apwojcik commented Oct 3, 2023 • edited Loading

apwojcik commented Oct 3, 2023

migraphx-bot commented Oct 4, 2023 • edited Loading

migraphx-bot commented Oct 4, 2023

pfultz2 commented Oct 4, 2023

codecov bot commented Oct 3, 2023 •

edited

Loading

apwojcik commented Oct 3, 2023 •

edited

Loading

migraphx-bot commented Oct 4, 2023 •

edited

Loading