Skip to content
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.

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

SCOPFLOW HiOp Patch #2

Closed
wants to merge 8 commits into from
Closed

SCOPFLOW HiOp Patch #2

wants to merge 8 commits into from

Conversation

jaelynlitz
Copy link
Contributor

@jaelynlitz jaelynlitz commented Sep 13, 2023

Initial Purpose: Fix divide by zero errors happening in SCOPFLOW - a hiop patch issue657-fix was added to resolve these issues. Just rebuilding with updated hiop and re-enabling those tests.
-> these errors were fixed in [email protected]

Current Purpose: build with hiop@develop (v1.0.1+) and resolve issues due to interface changes and compiler warnings.

Closes #3

@cameronrutherford
Copy link
Contributor

Moving to 1.6.1 since this will come with resolving #28 and other issues

@cameronrutherford
Copy link
Contributor

Can you kick off a rebuild as well so that we can use updated hiop@develop?

@nychiang
Copy link
Collaborator

@jaelynlitz Do those tests work now?

@cameronrutherford
Copy link
Contributor

There are a mix of HiOp and ExaGO build errors

@cameronrutherford cameronrutherford mentioned this pull request Nov 21, 2023
5 tasks
@jaelynlitz
Copy link
Contributor Author

@jaelynlitz Do those tests work now?

@nychiang It looks like on Deception and Newell, hiop built successfully, but there are some type errors in our SCOPFLOW hiop interface https://gitlab.pnnl.gov/exasgd/frameworks/exago-github-mirror/-/jobs/135161#L325

@jaelynlitz
Copy link
Contributor Author

jaelynlitz commented Nov 21, 2023

@nychiang Incline's build of hiop failed https://gitlab.pnnl.gov/exasgd/frameworks/exago-github-mirror/-/jobs/135160#L212 seems to be mostly unused variable warnings/errors

@jaelynlitz
Copy link
Contributor Author

@nychiang Ascent's hiop build failed with less errors, some syntax warnings and an include file it couldn't find https://code.ornl.gov/ecpcitest/exasgd/exago/-/jobs/2147169#L180

@cameronrutherford
Copy link
Contributor

@nychiang Ascent's hiop build failed with less errors, some syntax warnings and an include file it couldn't find https://code.ornl.gov/ecpcitest/exasgd/exago/-/jobs/2147169#L180

For some reason it seems like submodules aren't being cloned with develop (eigen header is missing, which is a submodule). You might have to update the spack package for HiOp so that develop clones submodules.

@cameronrutherford
Copy link
Contributor

@nychiang Incline's build of hiop failed https://gitlab.pnnl.gov/exasgd/frameworks/exago-github-mirror/-/jobs/135160#L212 seems to be mostly unused variable warnings/errors

This probably requires a HiOp patch then to work through those unused variables since HiOp is the one failing here

@cameronrutherford
Copy link
Contributor

@jaelynlitz Do those tests work now?

@nychiang It looks like on Deception and Newell, hiop built successfully, but there are some type errors in our SCOPFLOW hiop interface https://gitlab.pnnl.gov/exasgd/frameworks/exago-github-mirror/-/jobs/135161#L325

I think these are resolved by #30

@nychiang
Copy link
Collaborator

@jaelynlitz Do those tests work now?

@nychiang It looks like on Deception and Newell, hiop built successfully, but there are some type errors in our SCOPFLOW hiop interface https://gitlab.pnnl.gov/exasgd/frameworks/exago-github-mirror/-/jobs/135161#L325

Yes, as @cameronrutherford mentioned, this has been fixed in PR#30

@nychiang
Copy link
Collaborator

@nychiang Incline's build of hiop failed https://gitlab.pnnl.gov/exasgd/frameworks/exago-github-mirror/-/jobs/135160#L212 seems to be mostly unused variable warnings/errors

This probably requires a HiOp patch then to work through those unused variables since HiOp is the one failing here
We'd like to move "-Wall -Werror" flags to DEBUG mode, similar to ExaGO

@cameronrutherford
Copy link
Contributor

@nychiang Incline's build of hiop failed https://gitlab.pnnl.gov/exasgd/frameworks/exago-github-mirror/-/jobs/135160#L212 seems to be mostly unused variable warnings/errors

This probably requires a HiOp patch then to work through those unused variables since HiOp is the one failing here
We'd like to move "-Wall -Werror" flags to DEBUG mode, similar to ExaGO

@nychiang if you can make an issue in HiOp to track this that would be great so we can patch that in.

Perhaps there is a way to avoid these compiler issues at the spack level in the meantime...

@jaelynlitz
Copy link
Contributor Author

jaelynlitz commented Nov 22, 2023

@nychiang Incline's build of hiop failed https://gitlab.pnnl.gov/exasgd/frameworks/exago-github-mirror/-/jobs/135160#L212 seems to be mostly unused variable warnings/errors

This probably requires a HiOp patch then to work through those unused variables since HiOp is the one failing here
We'd like to move "-Wall -Werror" flags to DEBUG mode, similar to ExaGO

So only use those flags when building in Debug mode, ignore in Release mode?

I also disagree with some of the "unused" vars the compiler is catching - it says the MPI ierr flags are never used even though the next statement is an assert to check ierr?

https://github.com/LLNL/hiop/blob/develop/src/Utils/hiopLogger.hpp#L96C5-L96C5
https://gitlab.pnnl.gov/exasgd/frameworks/exago-github-mirror/-/jobs/135604#L468

@nychiang
Copy link
Collaborator

nychiang commented Nov 22, 2023

@nychiang Incline's build of hiop failed https://gitlab.pnnl.gov/exasgd/frameworks/exago-github-mirror/-/jobs/135160#L212 seems to be mostly unused variable warnings/errors

This probably requires a HiOp patch then to work through those unused variables since HiOp is the one failing here
We'd like to move "-Wall -Werror" flags to DEBUG mode, similar to ExaGO

So only use those flags when building in Debug mode, ignore in Release mode?

I also disagree with some of the "unused" vars the compiler is catching - it says the MPI ierr flags are never used even though the next statement is an assert to check ierr?

https://github.com/LLNL/hiop/blob/develop/src/Utils/hiopLogger.hpp#L96C5-L96C5 https://gitlab.pnnl.gov/exasgd/frameworks/exago-github-mirror/-/jobs/135604#L468

That happens when we define NDEBUG, which may be added by some compilers in the release mode. When NDEBUG is given, all assertion will be ignored (https://en.cppreference.com/w/c/error/assert). As a result, ierr becomes unused.

The best approach to handle this is to build our own error control, or use C++17 and add attribute maybe_unused ( https://en.cppreference.com/w/cpp/language/attributes/maybe_unused)

@jaelynlitz

@cameronrutherford
Copy link
Contributor

@nychiang Incline's build of hiop failed https://gitlab.pnnl.gov/exasgd/frameworks/exago-github-mirror/-/jobs/135160#L212 seems to be mostly unused variable warnings/errors

This probably requires a HiOp patch then to work through those unused variables since HiOp is the one failing here

We'd like to move "-Wall -Werror" flags to DEBUG mode, similar to ExaGO

So only use those flags when building in Debug mode, ignore in Release mode?

I also disagree with some of the "unused" vars the compiler is catching - it says the MPI ierr flags are never used even though the next statement is an assert to check ierr?

https://github.com/LLNL/hiop/blob/develop/src/Utils/hiopLogger.hpp#L96C5-L96C5 https://gitlab.pnnl.gov/exasgd/frameworks/exago-github-mirror/-/jobs/135604#L468

That happens when we define NDEBUG, which may be added by some compilers in the release mode. When NDEBUG is given, all assertion will be ignored (https://en.cppreference.com/w/c/error/assert). As a result, ierr becomes unused.

The best approach to handle this is to build our own error control, or use C++17 and add attribute maybe_unused ( https://en.cppreference.com/w/cpp/language/attributes/maybe_unused)

@jaelynlitz

Maybe unused works, and iirc there are gnu directives that are C++14 compatible that also work around the warnings.

@nychiang
Copy link
Collaborator

@cameronrutherford do you mean adding "(void) ierr;". This helps, but it is ugly in my opinion. :-)

@cameronrutherford
Copy link
Contributor

@cameronrutherford do you mean adding "(void) ierr;". This helps, but it is ugly in my opinion. :-)

That's one approach. There's a pre-processor directive I'm thinking of...

@pelesh probably has a better idea here :)

@abhyshr
Copy link
Collaborator

abhyshr commented Nov 22, 2023

Is this something because of PETSc? If so, I would recommend to use PETSc 3.20 and see if that resolves the issue. If not, let's send an email to PETSc mailing list. Satish or someone else from the PETSc team may be able to figure out what's happening here.

@cameronrutherford
Copy link
Contributor

Is this something because of PETSc? If so, I would recommend to use PETSc 3.20 and see if that resolves the issue. If not, let's send an email to PETSc mailing list. Satish or someone else from the PETSc team may be able to figure out what's happening here.

Alas this is isolated to HiOp and ExaGO :)

@jaelynlitz
Copy link
Contributor Author

closing as this was wrapped into #84

@jaelynlitz jaelynlitz closed this Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SCOPFLOW Functionality Tests failing with [email protected]
4 participants