Client improvements 2 #229

tfalders · 2021-03-08T23:38:24Z

Addresses SWDEV-275379.

Summary of proposed changes:

Improved emulation of boost's variables_map (I plan to port these changes over to rocBLAS and hipBLAS if accepted)
Convert Arguments class into a map of variables with easy-to-use get and set methods
- New argument variables only need to be added to client.cpp
- Getter method can return a default value if an actual value has not been provided
- Slight drawback in the form of more verbose code and increased compilation time
- Some argument variables (namely, test options) are still kept as public fields for ease-of-use

clients/rocblascommon/program_options.hpp

clients/include/testing_gesvd.hpp

clients/include/rocsolver_arguments.hpp

clients/benchmarks/client.cpp

clients/include/testing_getf2_getrf.hpp

jzuniga-amd

It looks good to me in general. Great job Troy and Cory!.
Below are some initial comments. I might have some more comments once the concept is applied to all functions...

My main concern now has to do with the error messages and the format they are printed. I have some suggestions to be more consistent with other messages, but it was easier to simply apply them in a PR to your branch than explain them here. So, please have a look and let me know what you think.

clients/benchmarks/client.cpp

clients/gtest/orgbr_ungbr_gtest.cpp

clients/include/rocsolver_arguments.hpp

* format messages and outputs * format messages and outputs * review corrections

cgmb

There's still a couple details to adjust, but it looks like a good change. Nice job!

clients/rocblascommon/program_options.hpp

clients/benchmarks/client.cpp

clients/gtest/bdsqr_gtest.cpp

clients/include/rocsolver_arguments.hpp

cgmb

Looks like the CI is having some trouble. Definitely wait for green checkmarks before merging.

~~I'm still not entirely behind the namespace choice, but that's small potatoes.~~

jzuniga-amd

Very nice Troy! The set and get mechanisms, and the mapping of all parameters is clean and functional...

I just have a few more suggested changes to clarify things from the user perspective and to avoid further errors with the default values.

jzuniga-amd · 2021-03-18T17:17:47Z

clients/benchmarks/client.cpp

+        ("k",
+         value<rocblas_int>()->default_value(128),
+            "Matrix/vector size parameter.\n"
+            "                           Typically, the number of Householder reflections in a transformation.\n"


Not sure yet of the description of this parameter...

I would say something like:
"Matrix/vector size parameter.
Represents a sub-dimension of a problem.
The number of Householder reflections in a transformation, for example."

Also, the default value of k cannot always be 128!! It depends on the problem... I would prefer not to assign a default.
For example, for orglq, k <= m... So, rocsolver-bench -f orglq -m 30 -n 30 will fail if k=128....!
The default should be calculated in the test body...
(also see other related comments below)

I don't think that calculating defaults for n and k in the test body will address this problem either. For example, rocsolver-bench -f orglq -k 150 will still fail since m is not calculated based on k. We should probably require the user to provide either all size arguments or none, so rocsolver-bench -f orglq -m 30 -n 30 -k 30 and rocsolver-bench -f orglq would be valid, but rocsolver-bench -f orglq -m 30 -n 30 and rocsolver-bench -f orglq -k 150 would not. What do you think?

Yeah, you are right! mmm I was hoping this not to get more complicated, but I don't like us providing default values that fail; better not to have defaults and always require the user to pass all parameters.

BTW what would happen if we call argus.get<rocblas_int>("m"); and m has no value?
If this method returns NAN or something that we can detect, then we can ask the user to always provide the "fundamental" parameter...

So, for example, as orglq computes a matrix with orthonormal rows, m must be provided; orgqr computes a matrix with orthogonal columns, so n must be provided, etc. Everything else can be calculated. (Most functions will do well with m=128, n=128, anyways).

So, in the case of rocsolver-bench -f orglq -k 150, we can maybe return an error saying
"A default value for -m cannot be calculated; please provide a suitable value" or something like this...

BTW what would happen if we call argus.get<rocblas_int>("m"); and m has no value?
If this method returns NAN or something that we can detect, then we can ask the user to always provide the "fundamental" parameter...

It would throw an std::out_of_range exception, which would trigger the program to terminate. I'm not sure if the error message would specifically state what argument was missing, but it might actually be pretty good already. If it's not good enough, we could clean that up a bit without changing it much. We'd define our own exception and if an argument was requested but not found, we'd do an explicit throw in get, then catch the exception in main() to print a nicer error message.

(I'm normally not a huge fan of exceptions as an error handling mechanism, but they do work fairly nicely for this sort of problem.)

So, in the case of rocsolver-bench -f orglq -k 150, we can maybe return an error saying
"A default value for -m cannot be calculated; please provide a suitable value" or something like this...

I like this idea. I think we can still keep the defaults for most functions, but for those you've mentioned I can calculate the secondary parameters and terminate the program if a secondary parameter is provided without the primary parameter.

If you agree with the idea of only require the "fundamental parameter(s)" for those problematic functions, here is a list of
[function - required parameter(s)]

laswp - k1
larft - n,k,storev
larf - m
larfb - m,n,k,side,storev
labrd - m,n
bdsqr - n,nu
latrd - n
steqr - n
orgxr/ungxr - m,n (yes, m is also required to avoid problems with lda)
orglx/unglx - m,n
orgxl/ungxl - m,n
orgbr/ungbr - m,n,storev
orgtr/ungtr - n
ormxr/unmxr - m,n,side
ormlx/unmlx - m,k,side
ormxl/unmxl - m,n,side
ormbr/unmbr - m,n,side,storev
ormtr/unmtr - m,n,side
potf2/potrf - n
getf2/getrf - m
geqr2/geqrf - m
geql2/geqlf - m
gelq2/gelqf - m
gebd2/gebrd - m
sytd2/sytrd/hetd2/hetrd - n
sygs2/sygst/hegs2/hegst - n
getri - n
getrs - n
gels - m,n
syev/heev - n
sygv/hegv - n
gesvd - m,n

So, rather almost always we will require the user to provide a size... but still we have some parameters that can default to something. It is mid-way between not requesting any parameter and requesting all parameters.
If you want, you can check whether the required parameter is really needed. For example if in potf2, 'n' is not provided, but neither 'lda', then both can get defaults... Even fancier, you can see whether one parameter can be calculated if we know the other, and so on. This can be as elaborated as we want. To me, always requesting the minimal parameters in the list is just fine.

As for the error, using the exceptions to provide the message is fine with me as long as it provides information on what is the missing required parameter.

Oh, you want the check on lda, ldb, etc. as well? That makes it a bit more involved... I'll have to think about how to do this cleanly...

Hmmm, I think we might be overengineering this a little bit. For now, adding calculations for the secondary parameters should be enough so that the client, in most cases, won't fail. I'll go ahead and do that. Perhaps we can discuss more advanced behaviours in a later meeting, as I also don't want things to become too complicated.

Oh, you want the check on lda, ldb, etc. as well? That makes it a bit more involved... I'll have to think about how to do this cleanly...

No, I am just saying that it could be done, but for me, it is fine if we just require the user to provide the primary parameters (as in the list), and calculate a default for the other parameters if not provided.

Ah, okay, that's more doable. I can add that in.

clients/benchmarks/client.cpp

jzuniga-amd · 2021-03-18T22:57:36Z

clients/include/testing_gebd2_gebrd.hpp

-    rocblas_stride stQ = argus.bsp;
-    rocblas_stride stP = argus.bsp;
+    rocblas_int m = argus.get<rocblas_int>("m");
+    rocblas_int n = argus.get<rocblas_int>("n");


Similarly,

rocblas_int m = argus.get<rocblas_int>("m", 128);
rocblas_int n = argus.get<rocblas_int>("n", 128);

Same for other functions below...

clients/include/testing_labrd.hpp

clients/include/testing_orglx_unglx.hpp

clients/include/testing_orgxl_ungxl.hpp

clients/include/testing_ormlx_unmlx.hpp

jzuniga-amd

LGTM.
A few non-important comments... except for LASWP which I think needs to be fixed.
After that, let's merge this PR! Good job...

jzuniga-amd · 2021-03-23T00:03:25Z

clients/include/testing_lacgv.hpp

 {
    // get arguments
    rocblas_local_handle handle;
-    rocblas_int n = argus.N;
-    rocblas_int inc = argus.incx;
+    rocblas_int n = argus.get<rocblas_int>("n");


n can take any value here. This could be
rocblas_int n = argus.get<rocblas_int>("n",128); if we want.

I'm uncomfortable with adding defaults that the user can't see. In cases where the default isn't a straightforward calculation, I'd prefer either no defaults (current situation) or global defaults (earlier situation, where the user can see these defaults in the help message). Defaulting to square matrices is a bit of an edge case, but we've now documented that in the help string.

Sounds good.

jzuniga-amd · 2021-03-23T00:08:19Z

clients/include/testing_larf.hpp

-    rocblas_int lda = argus.lda;
-    rocblas_int hot_calls = argus.iters;
-    char sideC = argus.side_option;
+    char sideC = argus.get<char>("side");


side can take any value here. This could be
char sideC = argus.get<char>("side",'L'); if we want.

jzuniga-amd · 2021-03-23T00:11:12Z

clients/include/testing_larfg.hpp

 {
    // get arguments
    rocblas_local_handle handle;
-    rocblas_int n = argus.N;
-    rocblas_int inc = argus.incx;
+    rocblas_int n = argus.get<rocblas_int>("n");


n can take any value here. This could be
rocblas_int n = argus.get<rocblas_int>("n",128); if we want.

jzuniga-amd · 2021-03-23T00:27:14Z

clients/include/testing_laswp.hpp

-    rocblas_int k2 = argus.k2;
-    rocblas_int inc = argus.incx;
+    rocblas_int n = argus.get<rocblas_int>("n");
+    rocblas_int lda = argus.get<rocblas_int>("lda", 1);


Although not required by the API, for the test to actually make sense, lda >= k2 otherwise you will be swapping non-existent rows. So, at least for the default cases, we need to ensure that no out of bond memory is accessed:

clients/staging/rocsolver-bench -f laswp -n 5 --k1 9 --k2 20
Memory access fault by GPU node-1 (Agent handle: 0x243ed30) on address 0x7f3b445ff000. Reason: Page not present or supervisor privilege.
Aborted (core dumped)

So, I guess we need
rocblas_int lda = argus.get<rocblas_int>("lda", k2);

And probably we should request k2 as well ?

Fixed.

Should we perhaps add a check for lda < k2 in laswp's argCheck as well? Maybe not in the PR, but if it's guaranteed to cause a memory access fault then that should probably be caught by the check.

The values we use in the gtest ensure we don't have problems, but users can easily get memory access faults (in code or using rocsolver-bench) if they are not careful...
LAPACK does not have a check for lda, but we can certainly add it in a future PR.

jzuniga-amd · 2021-03-23T00:30:03Z

clients/include/testing_laswp.hpp

-    rocblas_int k1 = argus.k1;
-    rocblas_int k2 = argus.k2;
-    rocblas_int inc = argus.incx;
+    rocblas_int n = argus.get<rocblas_int>("n");


n can take any value here. This could be
rocblas_int n = argus.get<rocblas_int>("n",128); if we want.

clients/include/testing_latrd.hpp

jzuniga-amd · 2021-03-23T00:40:56Z

clients/include/testing_sterf.hpp

 {
    // get arguments
    rocblas_local_handle handle;
-    rocblas_int n = argus.N;
+    rocblas_int n = argus.get<rocblas_int>("n");


n can take any value here. This could be
rocblas_int n = argus.get<rocblas_int>("n",128); if we want.

tfalders · 2021-03-23T17:28:31Z

If there are no objections, once CI passes I'll merge this PR.

tfalders added noCI Disable Jenkins for this PR. noOptimizations Disable optimized kernels for small sizes for some routines labels Mar 8, 2021

tfalders requested review from cgmb and jzuniga-amd March 8, 2021 23:38

tfalders added 3 commits March 8, 2021 17:12

Improved variables_map emulation

7e382fd

Prototype Arguments class

0d420a1

Prototype argument model for gesvd

fb6f7db

tfalders force-pushed the client_improvements2 branch from a7cc1c3 to fb6f7db Compare March 9, 2021 00:12

cgmb reviewed Mar 9, 2021

View reviewed changes

tfalders added 3 commits March 9, 2021 12:46

Addressed review comments

96d5ebf

Prototype argument model for sygv/hegv

3af61e9

Print names of arguments not consumed by tests

bca00a7

tfalders force-pushed the client_improvements2 branch from 0ce0852 to bca00a7 Compare March 9, 2021 22:27

cgmb reviewed Mar 10, 2021

View reviewed changes

clients/include/rocsolver_arguments.hpp Outdated Show resolved Hide resolved

doctorcolinsmith reviewed Mar 11, 2021

View reviewed changes

clients/benchmarks/client.cpp Show resolved Hide resolved

doctorcolinsmith reviewed Mar 11, 2021

View reviewed changes

clients/include/testing_getf2_getrf.hpp Show resolved Hide resolved

jzuniga-amd reviewed Mar 14, 2021

View reviewed changes

clients/benchmarks/client.cpp Outdated Show resolved Hide resolved

clients/benchmarks/client.cpp Outdated Show resolved Hide resolved

clients/gtest/orgbr_ungbr_gtest.cpp Show resolved Hide resolved

clients/include/rocsolver_arguments.hpp Outdated Show resolved Hide resolved

tfalders force-pushed the client_improvements2 branch from bca00a7 to d8293aa Compare March 15, 2021 17:18

Addressed review comment

dd417e4

tfalders force-pushed the client_improvements2 branch from d8293aa to dd417e4 Compare March 15, 2021 17:24

jzuniga-amd and others added 10 commits March 15, 2021 11:25

change format in messages and outputs (#5)

ee68711

* format messages and outputs * format messages and outputs * review corrections

Addressed review comments

438160b

New argument model for syev/heev

6bb3569

New argument model for sygst/hegst

b246120

New argument model for sytrd/hetrd

3801219

New argument model for potrf

7538d5c

New argument model for getrs

5dad3f9

New argument model for getri

c95c0fd

New argument model for getrf

7ce9308

New argument model for geqrf

be8014f

tfalders added 8 commits March 15, 2021 14:32

New argument model for orgxx functions

02a204b

New argument model for ormxx functions

55d0715

New argument model for HMM test

ac19b94

Merge branch 'client_improvements2_functions' into client_improvements2

7e22317

Final clean-up

bdfc41a

Consistent style for bench arguments

35e2d0f

Merge branch 'develop' into client_improvements2

fb4dafd

Updated changelog

957faad

tfalders removed the noCI Disable Jenkins for this PR. label Mar 15, 2021

tfalders marked this pull request as ready for review March 15, 2021 22:13

tfalders added 2 commits March 16, 2021 12:38

Alphabetize arguments

a3a7ed2

Bug fixes

5052e82

cgmb reviewed Mar 17, 2021

View reviewed changes

clients/rocblascommon/program_options.hpp Outdated Show resolved Hide resolved

clients/benchmarks/client.cpp Outdated Show resolved Hide resolved

clients/gtest/bdsqr_gtest.cpp Show resolved Hide resolved

clients/include/rocsolver_arguments.hpp Outdated Show resolved Hide resolved

tfalders added 2 commits March 17, 2021 17:48

Addressed review comments

b2d935d

Apply clang format

95e6909

cgmb approved these changes Mar 17, 2021

View reviewed changes

Addressed review comment

46ecba0

tfalders mentioned this pull request Mar 17, 2021

Client improvements ROCm/hipSOLVER#3

Merged

jzuniga-amd requested changes Mar 19, 2021

View reviewed changes

tfalders added 5 commits March 19, 2021 20:28

Addressed some review comments

b6dd5eb

Remove static defaults from m, n, k, storev, side, k1, k2, and nu

5064298

Default to square matrices

49fb0bc

Updated help string

a09fdef

Use m as required parameter instead of n

a171eb1

tfalders requested a review from jzuniga-amd March 22, 2021 17:36

jzuniga-amd approved these changes Mar 23, 2021

View reviewed changes

Adjust laswp and latrd defaults

c82ab9e

tfalders merged commit 12afdab into ROCm:develop Mar 23, 2021

tfalders mentioned this pull request Apr 9, 2021

Improved boost emulation ROCm/hipBLAS#331

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client improvements 2 #229

Client improvements 2 #229

tfalders commented Mar 8, 2021

jzuniga-amd left a comment

cgmb left a comment

cgmb left a comment •

edited

jzuniga-amd left a comment

jzuniga-amd Mar 18, 2021

tfalders Mar 19, 2021

jzuniga-amd Mar 19, 2021

cgmb Mar 19, 2021 •

edited

tfalders Mar 19, 2021 •

edited

jzuniga-amd Mar 19, 2021

tfalders Mar 19, 2021

tfalders Mar 19, 2021

jzuniga-amd Mar 19, 2021

tfalders Mar 19, 2021

jzuniga-amd Mar 18, 2021

jzuniga-amd left a comment

jzuniga-amd Mar 23, 2021

tfalders Mar 23, 2021

jzuniga-amd Mar 23, 2021

jzuniga-amd Mar 23, 2021

jzuniga-amd Mar 23, 2021

jzuniga-amd Mar 23, 2021

tfalders Mar 23, 2021

jzuniga-amd Mar 23, 2021

jzuniga-amd Mar 23, 2021

jzuniga-amd Mar 23, 2021

tfalders commented Mar 23, 2021

Client improvements 2 #229

Client improvements 2 #229

Conversation

tfalders commented Mar 8, 2021

jzuniga-amd left a comment

Choose a reason for hiding this comment

cgmb left a comment

Choose a reason for hiding this comment

cgmb left a comment • edited

Choose a reason for hiding this comment

jzuniga-amd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgmb Mar 19, 2021 • edited

Choose a reason for hiding this comment

tfalders Mar 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzuniga-amd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfalders commented Mar 23, 2021

cgmb left a comment •

edited

cgmb Mar 19, 2021 •

edited

tfalders Mar 19, 2021 •

edited