Teko: configuration error with deprecated code off and -D Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON #6355

ikalash · 2019-11-26T06:21:02Z

I attempted to switch Albany to turn off Tpetra and Xpetra deprecated code in the Albany nightlies (with @mperego). In some of the builds, I need to have the flag:

-D Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON \

in order to avoid DTK-related compilation errors. I also have Teko on, as it is used in some of the tests that use DTK. Doing so produces the following error:

Processing enabled package: Teko (Libs)
CMake Error at packages/teko/CMakeLists.txt:25 (MESSAGE):
  Teko currently requires that Tpetra enable either GlobalOrdinal (GO) = long
  long or GO = int.  Please set Tpetra_INST_INT_LONG_LONG=ON to enable GO =
  long long, or Tpetra_INST_INT_INT=ON to enable GO = int.  If you would like
  to use other GO types, please contact the Teko developers.

Is it possible to have a build with the following 4 options simultaneously?

 -D Tpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF \
 -D Xpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF \
 -D Trilinos_ENABLE_Teko:BOOL=ON \
 -D Tpetra_INST_INT_LONG_LONG=ON \

@trilinos/teko

The text was updated successfully, but these errors were encountered:

eric-c-cyr · 2019-11-26T14:13:25Z

Teko is probably hard coded to use one global ordinal type. I’m not really sure about changes propagated as part of the DEPRECATED refactor. I will do a quick check to see if there is something easy. Otherwise, I probably have no bandwidth for this until late January at the earliest.

…

Sent from my iPad On Nov 25, 2019, at 11:21 PM, Irina K. Tezaur <[email protected]> wrote: I attempted to switch Albany to turn off Tpetra and Xpetra deprecated code in the Albany nightlies (with @mperego<https://github.com/mperego>). In some of the builds, I need to have the flag:

-D Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON \ in order to avoid DTK-related compilation errors. I also have Teko on, as it is used in some of the tests that use DTK. Doing so produces the following error: Processing enabled package: Teko (Libs) CMake Error at packages/teko/CMakeLists.txt:25 (MESSAGE): Teko currently requires that Tpetra enable either GlobalOrdinal (GO) = long long or GO = int. Please set Tpetra_INST_INT_LONG_LONG=ON to enable GO = long long, or Tpetra_INST_INT_INT=ON to enable GO = int. If you would like to use other GO types, please contact the Teko developers. Is it possible to have a build with the following 4 options simultaneously? -D Tpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF \ -D Xpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF \ -D Trilinos_ENABLE_Teko:BOOL=ON \ -D Tpetra_INST_INT_LONG_LONG=ON \ @trilinos/teko<https://github.com/orgs/trilinos/teams/teko> — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub<#6355?email_source=notifications&email_token=ACHYSH3GN64Z2PVO4AUIODDQVS55FA5CNFSM4JRTC5QKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4H4AP24A>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACHYSH6HYT4G3EYLDBV3Y4LQVS55FANCNFSM4JRTC5QA>.

eric-c-cyr · 2019-11-26T14:24:06Z

After glancing at the code, I’m not sure what the issue is. I thought I understood what happened with the refactor, but clearly not. Can you speak to what Albany needs, and what you are trying to accomplish.

…

Sent from my iPad On Nov 26, 2019, at 7:13 AM, [email protected] wrote: Teko is probably hard coded to use one global ordinal type. I’m not really sure about changes propagated as part of the DEPRECATED refactor. I will do a quick check to see if there is something easy. Otherwise, I probably have no bandwidth for this until late January at the earliest.

Sent from my iPad On Nov 25, 2019, at 11:21 PM, Irina K. Tezaur <[email protected]> wrote: I attempted to switch Albany to turn off Tpetra and Xpetra deprecated code in the Albany nightlies (with @mperego<https://github.com/mperego>). In some of the builds, I need to have the flag:

-D Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON \ in order to avoid DTK-related compilation errors. I also have Teko on, as it is used in some of the tests that use DTK. Doing so produces the following error: Processing enabled package: Teko (Libs) CMake Error at packages/teko/CMakeLists.txt:25 (MESSAGE): Teko currently requires that Tpetra enable either GlobalOrdinal (GO) = long long or GO = int. Please set Tpetra_INST_INT_LONG_LONG=ON to enable GO = long long, or Tpetra_INST_INT_INT=ON to enable GO = int. If you would like to use other GO types, please contact the Teko developers. Is it possible to have a build with the following 4 options simultaneously? -D Tpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF \ -D Xpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF \ -D Trilinos_ENABLE_Teko:BOOL=ON \ -D Tpetra_INST_INT_LONG_LONG=ON \ @trilinos/teko<https://github.com/orgs/trilinos/teams/teko> — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub<#6355?email_source=notifications&email_token=ACHYSH3GN64Z2PVO4AUIODDQVS55FA5CNFSM4JRTC5QKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4H4AP24A>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACHYSH6HYT4G3EYLDBV3Y4LQVS55FANCNFSM4JRTC5QA>.

mhoemmen · 2019-11-26T16:42:06Z

@ikalash wrote:

-D Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON

Uh oh. I was under the impression that DTK no longer had that requirement. @rppawlo , would you happen to know if there is a way to tell DTK not to use unsigned long as the global index type?

@ikalash , if I remember right, the history is that DTK wanted to use the same global index type that STK uses. (My personal view is that there's no need for two interoperating packages to share index types. However, Tpetra's design circa 10 years ago unfortunately gave users the impression that they could and should change Tpetra's global index type to match that of any other library with which they wanted to interact. Sometimes design choices that intend "flexibility" end up causing rigidity instead.)

rppawlo · 2019-11-26T16:57:55Z

I haven't worked on DTK in a long time. Albany is using DTK version 2.0 that requires the unsigned long GO. They would have to upgrade to the latest DTK to use an arbitrary GO. cc'ing @sslattery to make sure that is still the case.

mperego · 2019-11-26T18:06:44Z

@mhoemmen can you see any workaround, or upgrading to the latest DTK is the only soltion?
@ikalash Is there any serious issue upgrading to the latest DTK?

mhoemmen · 2019-11-26T18:09:03Z

@ikalash Another thing you could try is disabling all the other GlobalOrdinal types, and just using unsigned long everywhere. Try this:

-D Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON
-D Tpetra_INST_INT_LONG:BOOL=OFF
-D Tpetra_INST_INT_LONG_LONG:BOOL=OFF
-D Tpetra_INST_INT_INT:BOOL=OFF

ikalash · 2019-11-26T19:01:25Z

Thanks for the suggestions. @mhoemmen , your suggestion does not fix the problem. Regarding @eric-c-cyr 's question:

I’m not really sure about changes propagated as part of the DEPRECATED refactor.

I was wondering the same thing. Can someone explain to me why turning off the deprecated code would cause this error to crop up, when it's not there w/o the deprecated code?

@rppawlo : I remember now past discussions about updating DTK. I tried it once and got a compilation error, so we kept the old DTK. Maybe we should look at upgrading it. CC'ing @lxmota for whom this issue would also be relevant.

rppawlo · 2019-11-26T19:11:42Z

tpetra now only allows a single GO that is picked at configure time. When you turn off deprecated code, you are stuck with choosing one GO. Albany has always built with at least two GOs. Unfortunately DTK 2.0 requires unsigned long and it looks like that GO isn't working for part of the trilinos stack. You could just try removing the teko configure check at packages/teko/CMakeLists.txt:25 and see if the build works. Maybe teko required a specific GO at one point but now works with any. Maybe the check is no longer needed?

having to do with DTK. Seems we will need to switch to newer DTK which will require some effort. Please see trilinos/Trilinos#6355 for more details.

eric-c-cyr · 2019-11-26T20:38:08Z

Yes, it seems like there is only a single GO used in Teko. I feel we could probably just snag the one from Tpetra at this point.

…

Sent from my iPad On Nov 26, 2019, at 12:11 PM, Roger Pawlowski <[email protected]> wrote: tpetra now only allows a single GO that is picked at configure time. When you turn off deprecated code, you are stuck with choosing one GO. Albany has always built with at least two GOs. Unfortunately DTK 2.0 requires unsigned long and it looks like that GO isn't working for part of the trilinos stack. You could just try removing the teko configure check at packages/teko/CMakeLists.txt:25 and see if the build works. Maybe teko required a specific GO at one point but now works with any. Maybe the check is no longer needed? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#6355?email_source=notifications&email_token=ACHYSH3NW6YIORFLWIZ3DHDQVVYG7A5CNFSM4JRTC5QKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFHD75Q#issuecomment-558776310>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACHYSHYD27IHMPSZF7ADHW3QVVYG7ANCNFSM4JRTC5QA>.

mhoemmen · 2019-11-26T21:18:11Z

@ikalash wrote:

@mhoemmen , your suggestion does not fix the problem.

Please post the resulting errors.

ikalash · 2019-11-27T03:26:02Z

@mhoemmen : same errors as before from Teko with your options + the deprecated code options:

CMake Error at packages/teko/CMakeLists.txt:25 (MESSAGE):
  Teko currently requires that Tpetra enable either GlobalOrdinal (GO) = long
  long or GO = int.  Please set Tpetra_INST_INT_LONG_LONG=ON to enable GO =
  long long, or Tpetra_INST_INT_INT=ON to enable GO = int.  If you would like
  to use other GO types, please contact the Teko developers.

Here are the options I have:

 -D Tpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF \
 -D Xpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF \
-D Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON \
      -D Tpetra_INST_INT_LONG:BOOL=OFF \
      -D Tpetra_INST_INT_LONG_LONG:BOOL=OFF \
      -D Tpetra_INST_INT_INT:BOOL=OFF \

eric-c-cyr · 2019-11-27T04:31:40Z

Perhaps we comment out the offending code in the teko/CMakeLists.txt file. Then in teko/src/Teko_ConfigDefs.hpp define the GO to be whatever is required from Tpetra (not sure what the line is, suggest asking @mhoemmen<https://github.com/mhoemmen>) From: "Irina K. Tezaur" <[email protected]> Reply-To: trilinos/Trilinos <[email protected]> Date: Tuesday, November 26, 2019 at 8:26 PM To: trilinos/Trilinos <[email protected]> Cc: "Cyr, Eric C" <[email protected]>, Mention <[email protected]> Subject: [EXTERNAL] Re: [trilinos/Trilinos] Teko: configuration error with deprecated code off and -D Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON (#6355) @mhoemmen<https://github.com/mhoemmen> : same errors as before from Teko with your options + the deprecated code options: CMake Error at packages/teko/CMakeLists.txt:25 (MESSAGE): Teko currently requires that Tpetra enable either GlobalOrdinal (GO) = long long or GO = int. Please set Tpetra_INST_INT_LONG_LONG=ON to enable GO = long long, or Tpetra_INST_INT_INT=ON to enable GO = int. If you would like to use other GO types, please contact the Teko developers. Here are the options I have: -D Tpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF \ -D Xpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF \

…

-D Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON \ -D Tpetra_INST_INT_LONG:BOOL=OFF \ -D Tpetra_INST_INT_LONG_LONG:BOOL=OFF \ -D Tpetra_INST_INT_INT:BOOL=OFF \ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#6355?email_source=notifications&email_token=ACHYSH5BKNIA67IRRG4AFLTQVXSE5A5CNFSM4JRTC5QKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFIFCQA#issuecomment-558911808>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACHYSH4G2DYFFSV3VSZQGXDQVXSE5ANCNFSM4JRTC5QA>.

mhoemmen · 2019-11-27T05:38:16Z

@eric-c-cyr wrote:

...then in teko/src/Teko_ConfigDefs.hpp define the GO to be whatever is required from Tpetra (not sure what the line is, suggest asking @mhoemmen)

That header file already includes Tpetra_Map_decl.hpp, so you can just get the default GO from Map, in the same way that the file already gets the default NT from Map.

namespace Teko {
  using LO = Tpetra::Map<>::local_ordinal_type;
  using GO = Tpetra::Map<>::global_ordinal_type;
  using NT = Tpetra::Map<>::node_type;
}

(Note the C++11 type alias syntax; it should replace all use of typedef in new code.) No macros, no errors, no fuss.

eric-c-cyr · 2019-11-27T05:41:15Z

That looks exactly right, @ikalash<https://github.com/ikalash> is this something you could try? I can help over the phone/skype (maybe on Friday). You can always send a patch too. From: Mark Hoemmen <[email protected]> Reply-To: trilinos/Trilinos <[email protected]> Date: Tuesday, November 26, 2019 at 10:38 PM To: trilinos/Trilinos <[email protected]> Cc: "Cyr, Eric C" <[email protected]>, Mention <[email protected]> Subject: [EXTERNAL] Re: [trilinos/Trilinos] Teko: configuration error with deprecated code off and -D Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON (#6355) @eric-c-cyr<https://github.com/eric-c-cyr> wrote: ...then in teko/src/Teko_ConfigDefs.hpp define the GO to be whatever is required from Tpetra (not sure what the line is, suggest asking @mhoemmen<https://github.com/mhoemmen>) That header file already includes Tpetra_Map_decl.hpp, so you can just get the default GO from Map, in the same way that you already get the default node_type from Map. namespace Teko { using LO = Tpetra::Map<>::local_ordinal_type; using GO = Tpetra::Map<>::global_ordinal_type; using NT = Tpetra::Map<>::node_type; } Note the C++11 type alias syntax; it should replace all use of typedef in new code. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#6355?email_source=notifications&email_token=ACHYSH76TY4RZ65BNIQBTTTQVYBUTA5CNFSM4JRTC5QKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFILMDA#issuecomment-558937612>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACHYSH6IZGEZGKQORSVAOI3QVYBUTANCNFSM4JRTC5QA>.

ikalash · 2019-11-27T07:48:10Z

@mhoemmen , @eric-c-cyr : thanks for the suggestions! Yes I can give this a try. My parents are in town for Thanksgiving so I don't know if I'll get to it this week. If not, definitely Monday. I imagine if I implement the suggestion and it fixes the problem, I can submit a PR to get it into the main code?

eric-c-cyr · 2019-11-27T14:20:00Z

Please do so that would be a great help. I may be try this while digesting this weekend and if I do I’ll send you a patch directly. From: "Irina K. Tezaur" <[email protected]> Reply-To: trilinos/Trilinos <[email protected]> Date: Wednesday, November 27, 2019 at 12:48 AM To: trilinos/Trilinos <[email protected]> Cc: "Cyr, Eric C" <[email protected]>, Mention <[email protected]> Subject: [EXTERNAL] Re: [trilinos/Trilinos] Teko: configuration error with deprecated code off and -D Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON (#6355) @mhoemmen<https://github.com/mhoemmen> , @eric-c-cyr<https://github.com/eric-c-cyr> : thanks for the suggestions! Yes I can give this a try. My parents are in town for Thanksgiving so I don't know if I'll get to it this week. If not, definitely Monday. I imagine if I implement the suggestion and it fixes the problem, I can submit a PR to get it into the main code? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#6355?email_source=notifications&email_token=ACHYSH6XVHNMN25COY432K3QVYQ3XA5CNFSM4JRTC5QKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFITRIQ#issuecomment-558971042>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACHYSH5UZ3YPF54PPXX73QLQVYQ3XANCNFSM4JRTC5QA>.

ikalash · 2019-11-27T17:16:45Z

Sounds like a plan! I’ll keep you posted.

ikalash · 2019-12-07T01:51:31Z

@eric-c-cyr , @mhoemmen : sorry about the delay - I finally had a chance to try your proposed fix, namely modify Teko_ConfigDefs.hpp to look as follows:

namespace Teko {

typedef double ST;
using LO = Tpetra::Map<>::local_ordinal_type;
using GO = Tpetra::Map<>::global_ordinal_type;
using NT = Tpetra::Map<>::node_type;
}

and make some changes to get the Teko tests/examples to compile and not show warnings when compiled. Unfortunately this leads to some failures in Albany. Some of them seem to have to do with us not allocating large enough graphs for some problems involving DTK/our Schwarz coupling method, which is not your problem, but I noticed a few other failures namely this one:

3: ************************************************************************
3: -- Nonlinear Solver Step 0 -- 
3: ||F|| = 3.633e+08  step = 0.000e+00  dx = 0.000e+00
3: ************************************************************************
3: 
3:        CALCULATING FORCING TERM
3:        Method: Constant
3:        Forcing Term: 0.0001
3: 
3: p=2: *** Caught standard std::exception of type 'std::logic_error' :
3: 
3:  /home/ikalash/nightlyAlbanyTests/Results/Trilinos/build/install/include/Tpetra_ComputeGatherMap.hpp:84:
3:  
3:  Throw number = 1
3:  
3:  Throw test that evaluated to true: true
3:  
3:  Not implemented for IntType != int, long, or long long

It looks like a routine in Tpetra_ComputeGatherMap.h is being called with unsigned int long, which is not supported. I'm a bit confused as to why my changes to Teko would produce this error. Without the changes, the test runs correctly (even with the deprecated Tpetra/Xpetra code off, not on my machine, which has DTK on, but on other machines, where DTK is off, so I can build everything just fine). Thoughts? BTW, I'm in ABQ next week so we could look at this together sometime if it's easier and you have time.

eric-c-cyr · 2019-12-07T02:14:49Z

Possibly in the matrix multiply? Does your preconditioner do any matrix multiplies or additions in setup? If you have a debug build you could do an assertion check if its not to costly. I’ll be here a bit on Monday and Tuesday. But my free time is limited. If you want to meet we should schedule something.

…

Sent from my iPad On Dec 6, 2019, at 6:51 PM, Irina K. Tezaur <[email protected]> wrote: @eric-c-cyr<https://github.com/eric-c-cyr> , @mhoemmen<https://github.com/mhoemmen> : sorry about the delay - I finally had a chance to try your proposed fix, namely modify Teko_ConfigDefs.hpp to look as follows: namespace Teko { typedef double ST; using LO = Tpetra::Map<>::local_ordinal_type; using GO = Tpetra::Map<>::global_ordinal_type; using NT = Tpetra::Map<>::node_type; and make some changes to get the Teko tests/examples to compile and not show warnings when compiled. Unfortunately this leads to some failures in Albany. Some of them seem to have to do with us not allocating large enough graphs for some problems involving DTK/our Schwarz coupling method, which is not your problem, but I noticed a few other failures namely this one: 3: ************************************************************************ 3: -- Nonlinear Solver Step 0 -- 3: ||F|| = 3.633e+08 step = 0.000e+00 dx = 0.000e+00 3: ************************************************************************ 3: 3: CALCULATING FORCING TERM 3: Method: Constant 3: Forcing Term: 0.0001 3: 3: p=2: *** Caught standard std::exception of type 'std::logic_error' : 3: 3: /home/ikalash/nightlyAlbanyTests/Results/Trilinos/build/install/include/Tpetra_ComputeGatherMap.hpp:84: 3: 3: Throw number = 1 3: 3: Throw test that evaluated to true: true 3: 3: Not implemented for IntType != int, long, or long long It looks like a routine in Tpetra_ComputeGatherMap.h is being called with unsigned int long, which is not supported. I'm a bit confused as to why my changes to Teko would produce this error. Without the changes, the test runs correctly (even with the deprecated Tpetra/Xpetra code off, not on my machine, which has DTK on, but on other machines, where DTK is off, so I can build everything just fine). Thoughts? BTW, I'm in ABQ next week so we could look at this together sometime if it's easier and you have time. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#6355?email_source=notifications&email_token=ACHYSHZQ6GB3OZBXJRDL35DQXL6SJA5CNFSM4JRTC5QKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGF24YY#issuecomment-562802275>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACHYSH5LAL4RSK27BOC7REDQXL6SJANCNFSM4JRTC5QA>.

ikalash · 2019-12-07T03:16:15Z

Just to be clear, the tests with error message don't use teko. That's what I don't understand...

mhoemmen · 2019-12-09T18:32:52Z

@ikalash This is why we don't want to support random GlobalOrdinal types ;-) Trilinos doesn't test GO=unsigned long. I can try to fix this by converting the indices if they are of an unsupported type, but it will take a little while. This is technically a @trilinos/tpetra issue, but I'd rather that we stop using poorly tested GO types.

ikalash · 2019-12-09T19:28:28Z

@mhoemmen : I agree that at this level it's a @trilinos/tpetra issue. It seems to me that the current behavior is not ideal, if the code allows you to build with the unsigned long type (which I presume it would even w/o my change to Teko, if Teko were disabled I think), but then you may get run-time errors. If you prefer not to support types like GO=unsigned long, I think the best thing to do would be to throw a configure-time error when trying to use the type (like Teko does now, I guess). That of course would not fix my issue, which requires me to use ``GO=unsigned long" for DTK... the fix you propose to convert indices to the currently unsupported type (hopefully) would.

I've started looking into switching to a newer DTK, but am stuck for the moment. I'm getting various compilation errors that don't seem to have anything to do with Albany - I've opened a DTK issue on this and am waiting to hear back from Stuart Slattery on it.

kddevin · 2019-12-09T19:49:40Z

The unimplemented routine is getMpiDatatype
Would there be any harm in our adding an instantiation for MPI_UNSIGNED_LONG?
Would that be sufficient to solve the issue (without any sort of conversions)?

ikalash · 2019-12-09T19:54:12Z

@kddevin : that would be great if there's a temporary fix while I figure out how to get the newer DTK versions (which should build with a different GO than unsigned long) to work.

If you have a PR/snippet of code I can hack into my tpetra version with the additional getMpiDataType routine, I can test if that's sufficient. It's conceivable other issues will crop up with this fix that we're not aware of now, but hopefully not.

kddevin · 2019-12-09T20:03:29Z

Thanks for being willing to try a patch, @ikalash .
Here's what I would try.
Add near line 94, still inside the HAVE_MPI ifdef block:

      template<> TRILINOS_UNUSED_FUNCTION MPI_Datatype
      getMpiDatatype<unsigned long> () { return MPI_UNSIGNED_LONG; }

Sorry I don't have a way to reproduce your error and test this for you.

ikalash · 2019-12-09T20:05:54Z

Thanks @kddevin! I'll test it and get back to you. I wasn't expecting you to reproduce/test the fix - happy to do that myself. I'll post back once I've had a chance to do it.

mhoemmen · 2019-12-09T20:29:22Z

@kddevin Thanks -- I'll check whether MPI_UNSIGNED_LONG comes with MPI 3 by default.

mhoemmen · 2019-12-09T20:32:54Z

@kddevin The MPI 3 standard includes MPI_UNSIGNED, MPI_UNSIGNED_LONG, and MPI_UNSIGNED_LONG_LONG. I'm pretty sure we require MPI 3 for testing now and don't need to support (say) OpenMPI 1.6, so I think it's safe to use -- thanks!

ikalash · 2019-12-11T18:33:15Z

@kddevin : sorry for the delay in my response - I'm in a workshop for most of this week. It appears your proposed fix resolves my issue. Per Mark's comment, do we want to include the unsigned and unsigned long long version of getMpiDataType too?

Also, should I create a PR for the Tpetra (and Teko) changes? I am still doing some testing before this would be ready, in particular, it appears my changes broke a couple of the Teko tests, which I need to investigate/fix.

…ossible to compile Trilinos with Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON . There are still some issues to be addressed.

ikalash · 2019-12-15T18:24:08Z

@kddevin , @mhoemmen , @eric-c-cyr : I created a branch in which I implemented the proposed changes by the three of you above: https://github.com/trilinos/Trilinos/tree/ikalash-tpetra-teko-unsigned-long . I'm concerned about some of the logic throughout Teko/Tpetra that may not be right with GO=unsigned long. I'm attaching the output when I build Trilinos with my changes. You can see there are a lot of -Wsign-compare warnings, and I'm not sure all of them are innocuous. In particular, it looks like certain variables representing GIDs are allowed to be negative and there is logic to handle that case, e.g.:

/home/ikalash/dtkBuilds/TrilinosDtk2-ikalash-tpetra-teko-unsigned-long/packages/teko/src/Tpetra/Teko_BlockingTpetra.cpp:272:16: warning: comparison of integer expressions of different signedness: ‘Teko::GO’ {aka ‘long unsigned int’} and ‘int’ [-Wsign-compare]
  272 |          if(gid==-1) continue; // in contiguous row

and

/home/ikalash/dtkBuilds/TrilinosDtk2-ikalash-tpetra-teko-unsigned-long/packages/tpetra/core/src/Tpetra_Import_Util.hpp:432:15: warning: comparison of integer expressions of different signedness: ‘long unsigned int’ and ‘int’ [-Wsign-compare]
  432 |       if(cgid == -2) continue;

I suppose this would have been an issue in the develop branch with -D Tpetra_INST_INT_LONG_LONG=ON and -D Tpetra_ENABLE_DEPRECATED_CODE:BOOL=ON , -D Xpetra_ENABLE_DEPRECATED_CODE:BOOL=ON.

Also, there are some test failures in Tpetra:

The following tests FAILED:
         99 - TpetraCore_ImportExport_UnitTests_MPI_4 (Failed)
        111 - TpetraCore_ImportExport2_UnitTests_MPI_4 (Failed)
        112 - TpetraCore_ImportExport2_CrsSortingUtils_MPI_4 (Failed)
        142 - TpetraCore_MatrixMatrix_UnitTests_MPI_4 (Failed)

These have to do with the issue I describe above, I think - I fixed the HashTable test by commenting out the key=-1 case (which would be invalid for GO=unsigned long): 2dc1802#diff-936bd1d31c5c99477f4948a8d1398d95 . This probably should be reworked. Would you be able to have a look at it?

make_out.txt

ikalash · 2019-12-15T19:08:19Z

Just to follow up on my previous comment - I get a bunch of Tpetra test failures when building the develop branch (w/o any of my changes) with the options

 -D Tpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF \
 -D Xpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF \
 -D Trilinos_ENABLE_Teko:BOOL=ON \
 -D Tpetra_INST_INT_LONG_LONG=ON \

It's pretty much the same failures:

The following tests FAILED:
         96 - TpetraCore_FixedHashTableTest_MPI_1 (Failed)
         99 - TpetraCore_ImportExport_UnitTests_MPI_4 (Failed)
        112 - TpetraCore_ImportExport2_UnitTests_MPI_4 (Failed)
        113 - TpetraCore_ImportExport2_CrsSortingUtils_MPI_4 (Failed)
        114 - TpetraCore_MatrixMarket_Tpetra_CrsMatrix_InOutTest_MPI_4 (Failed)
        115 - TpetraCore_MatrixMarket_Tpetra_CrsGraph_InOutTest_MPI_4 (Failed)
        116 - TpetraCore_MatrixMarket_Operator_Test_MPI_4 (Failed)
        147 - TpetraCore_MatrixMatrix_UnitTests_MPI_4 (Failed)

So I guess the -D Tpetra_INST_INT_LONG_LONG=ON option leads to test failures. I'm ok with this if you're OK with this... but I thought I should bring it up.

mhoemmen · 2019-12-16T05:08:10Z

@ikalash wrote:

I get a bunch of Tpetra test failures when building the develop branch (w/o any of my changes) with the options....

Did you actually mean to write Tpetra_INST_INT_LONG_LONG=ON, or did you mean to write Tpetra_INST_INT_UNSIGNED_LONG=ON? I'm asking because if you disable deprecated code in Tpetra, the default GlobalOrdinal type is long long. That would make Tpetra_INST_INT_LONG_LONG=ON redundant.

ikalash · 2019-12-16T05:16:15Z

@mhoemmen : no, I meant Tpetra_INST_INT_UNSIGNED_LONG=ON. I need to set this to get the codes to compile with the version of DTK (v2) I am using (newer versions of DTK don't require this but I'm having some trouble building with them, so I am sticking with DTK v2 for now). Essentially what I'm finding is some Tpetra tests fail with the option Tpetra_INST_INT_UNSIGNED_LONG=ON in master/develop as well as my branch (which has some changes to get Teko, Tpetra and DTK to play nicely with Tpetra_INST_INT_UNSIGNED_LONG=ON and the deprecated Tpetra/Xpetra code turned off). This has probably been the case all along, I just never ran Tpetra tests.

mhoemmen · 2019-12-16T05:49:29Z

@ikalash wrote:

Essentially what I'm finding is some Tpetra tests fail with the option Tpetra_INST_INT_UNSIGNED_LONG=ON in master/develop as well as my branch (which has some changes to get Teko, Tpetra and DTK to play nicely with Tpetra_INST_INT_UNSIGNED_LONG=ON and the deprecated Tpetra/Xpetra code turned off).

This is ... unsurprising, given that the automatic PR tests have never enabled GlobalOrdinal=unsigned long. I don't think there's even a Dashboard test that does so. DTK folks requested this feature long ago; Trilinos devs added it very reluctantly and only if DTK folks promised to maintain and test it themselves. I can't speak for @trilinos/tpetra as a whole, but I imagine Trilinos developers wouldn't mind fixing obvious issues like the ones you pointed out. Long-term support is a bit trickier. Would you need DTK v2 to work for a while, or is this a temporary measure until you can upgrade to a newer DTK version?

ikalash · 2019-12-16T06:27:56Z

@mhoemmen : thanks for the explanations. I do need DTK v2 to work for awhile (a few months, maybe?). I'm still trying to figure out what the issues I'm having with DTK v3 and newer are being caused by. Could we merge my PR in while I'm try to get the code to work with the newer DTK?

mhoemmen · 2019-12-16T06:31:23Z

@ikalash Did you actually make a PR out of the branch https://github.com/trilinos/Trilinos/tree/ikalash-tpetra-teko-unsigned-long ? or are you referring to a different PR?

ikalash · 2019-12-16T06:32:28Z

Yes, exactly, that is the PR I'm referring to. I added you as one of the reviewers.

github-actions · 2021-11-07T12:24:27Z

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity.
If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label.
If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE.
If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

github-actions · 2021-12-08T12:27:27Z

This issue was closed due to inactivity for 395 days.

ikalash added type: bug The primary issue is a bug in Trilinos code or tests pkg: Teko client: Albany Issue impacting the Albany project labels Nov 26, 2019

ikalash added a commit that referenced this issue Dec 15, 2019

Teko, Tpetra: implementing proposed fixes in issue #6355 to make it p…

2dc1802

…ossible to compile Trilinos with Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON . There are still some issues to be addressed.

ikalash mentioned this issue Dec 15, 2019

LCM: get Albany + DTK to compile with deprecated Tpetra/Xpetra code OFF sandialabs/Albany#544

Closed

github-actions bot added the MARKED_FOR_CLOSURE Issue or PR is marked for auto-closure by the GitHub Actions bot. label Nov 7, 2021

github-actions bot added the CLOSED_DUE_TO_INACTIVITY Issue or PR has been closed by the GitHub Actions bot due to inactivity. label Dec 8, 2021

github-actions bot closed this as completed Dec 8, 2021

Teko: configuration error with deprecated code off and -D Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON #6355

Teko: configuration error with deprecated code off and -D Tpetra_INST_INT_UNSIGNED_LONG:BOOL=ON #6355

Comments

ikalash commented Nov 26, 2019

eric-c-cyr commented Nov 26, 2019 via email

eric-c-cyr commented Nov 26, 2019 via email

mhoemmen commented Nov 26, 2019 • edited Loading

rppawlo commented Nov 26, 2019

mperego commented Nov 26, 2019

mhoemmen commented Nov 26, 2019

ikalash commented Nov 26, 2019

rppawlo commented Nov 26, 2019

eric-c-cyr commented Nov 26, 2019 via email

mhoemmen commented Nov 26, 2019

ikalash commented Nov 27, 2019

eric-c-cyr commented Nov 27, 2019 via email

mhoemmen commented Nov 27, 2019 • edited Loading

eric-c-cyr commented Nov 27, 2019 via email

ikalash commented Nov 27, 2019

eric-c-cyr commented Nov 27, 2019 via email

ikalash commented Nov 27, 2019

ikalash commented Dec 7, 2019 • edited Loading

eric-c-cyr commented Dec 7, 2019 via email

ikalash commented Dec 7, 2019

mhoemmen commented Dec 9, 2019

ikalash commented Dec 9, 2019 • edited Loading

kddevin commented Dec 9, 2019

ikalash commented Dec 9, 2019 • edited Loading

kddevin commented Dec 9, 2019

ikalash commented Dec 9, 2019

mhoemmen commented Dec 9, 2019

mhoemmen commented Dec 9, 2019

ikalash commented Dec 11, 2019

ikalash commented Dec 15, 2019

ikalash commented Dec 15, 2019

mhoemmen commented Dec 16, 2019

ikalash commented Dec 16, 2019

mhoemmen commented Dec 16, 2019

ikalash commented Dec 16, 2019

mhoemmen commented Dec 16, 2019 • edited Loading

ikalash commented Dec 16, 2019

github-actions bot commented Nov 7, 2021

github-actions bot commented Dec 8, 2021

mhoemmen commented Nov 26, 2019 •

edited

Loading

mhoemmen commented Nov 27, 2019 •

edited

Loading

ikalash commented Dec 7, 2019 •

edited

Loading

ikalash commented Dec 9, 2019 •

edited

Loading

ikalash commented Dec 9, 2019 •

edited

Loading

mhoemmen commented Dec 16, 2019 •

edited

Loading