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

Remove support for PETSc < 3.5 #19197

Merged
merged 3 commits into from
Oct 26, 2021
Merged

Remove support for PETSc < 3.5 #19197

merged 3 commits into from
Oct 26, 2021

Conversation

dschwen
Copy link
Member

@dschwen dschwen commented Oct 25, 2021

Closes #19195

@moosebuild
Copy link
Contributor

moosebuild commented Oct 25, 2021

Job Documentation on 50cf97b wanted to post the following:

View the site here

This comment will be updated on new commits.

@cticenhour
Copy link
Member

cticenhour commented Oct 25, 2021

After this is merged, could you make a note of this in #19174?

EDIT: nevermind, I think we'll merge October a little early and allow folks to enter entries in their own PRs through the end of the month.

Copy link
Member

@cticenhour cticenhour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please modify affected petsc_version parameters in tests to be >= 3.5.0 where applicable as well. I see only a few with a quick search.

Copy link
Contributor

@fdkong fdkong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have something like this in Moose.h

#ifndef LIBMESH_HAVE_PETSC
#error PETSc has not been detected, please ensure your environment is set up properly then rerun the libmesh build script and try to compile MOOSE again.
#endif

I think we should add a similar thing for this change as well:

#if PETSC_VERSION_LESS_THAN(3, 5, 0)
#error PETSc-3.5.x or newer is required, please ensure your environment have the required version of PETSc
#endif 

@dschwen
Copy link
Member Author

dschwen commented Oct 25, 2021

I think we should add a similar thing for this change as well:

I can add this but I don't know how users would ever hit this error if they cannot even build libmesh with PETSc < 3.5

@dschwen
Copy link
Member Author

dschwen commented Oct 25, 2021

Am I right in assuming that petsc_version_release = true is very circumstantial? I.e. this refers to a very specific reeas of an old version.

@moosebuild
Copy link
Contributor

moosebuild commented Oct 25, 2021

Job Generate and verify coverage on 50cf97b wanted to post the following:

Framework coverage

0eb58f #19197 50cf97
Total Total +/- New
Rate 81.80% 81.80% +0.00% 80.00%
Hits 71498 71498 - 4
Misses 15912 15911 -1 1

Diff coverage report

Full coverage report

Modules coverage

Contact

0eb58f #19197 50cf97
Total Total +/- New
Rate 78.17% 78.17% - 100.00%
Hits 2202 2202 - 1
Misses 615 615 - 0

Diff coverage report

Full coverage report

Full coverage reports

Reports

Warnings

  • framework new line coverage rate 80.00% is less than the suggested 90.0%

This comment will be updated on new commits.

@cticenhour
Copy link
Member

cticenhour commented Oct 25, 2021

I believe petsc_version_release = true only ensures the test isn't run on PETSc-master. petsc_version_release = false tests against current master, i.e. outside of a current release.

@dschwen
Copy link
Member Author

dschwen commented Oct 25, 2021

Yeah, but a petsc_version_release = true will quickly lose its meaning because master moves on fast.

@cticenhour
Copy link
Member

cticenhour commented Oct 25, 2021

Looking at PETSc source, it seems that having the parameter set to true would also skip a test on any non-release branch (since PETSC_VERSION_RELEASE is always 0 unless you are on the release branch). See https://gitlab.com/petsc/petsc/-/blob/main/include/petscversion.h

@dschwen
Copy link
Member Author

dschwen commented Oct 25, 2021

Yeah, this seems pretty clear. My point here is that this condition probably makes sense in a narrow time window around when that minimum required version is first available in master. Two years later the reason for choosing release over master is most likely irrelevant because both branches have moved on so far.

@cticenhour
Copy link
Member

cticenhour commented Oct 25, 2021

Yes, I agree. I was just pointing out how limited our current description in run_tests is, but then again that description is applicable to how we would be using it in properly setting up a cutting-edge test spec.

For suitably old tests, they should probably be removed. We have some more recent ones where it might still be applicable.

@lindsayad
Copy link
Member

I don’t know that libmesh has a minimum supported PETSc version so I think the error in MOOSE makes sense

@dschwen
Copy link
Member Author

dschwen commented Oct 25, 2021

I don’t know that libmesh has a minimum supported PETSc version so I think the error in MOOSE makes sense

@roystgnr mentioned on slack that support for PETSc < 3.5 was cut from libmesh in February.

Copy link
Member

@cticenhour cticenhour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comments were addressed. @fdkong, do you have any more?

@dschwen dschwen merged commit 86b85ef into idaholab:next Oct 26, 2021
MatColoringDestroy(&mc);
#endif
ISColoringDestroy(&iscoloring);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the problem @dschwen

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If PESTC version is _not _ less than 3.5 use MatColoringDestroy... so this looks right to me. ISColoringDestroy seems to be the legacy call. But this is where I looked first. It has to be related to this function, and in particular the teardown of the datastructures here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤡

dschwen added a commit to dschwen/moose that referenced this pull request Oct 27, 2021
yjung-anl pushed a commit to yjung-anl/moose that referenced this pull request Oct 28, 2021
cticenhour added a commit to cticenhour/moose that referenced this pull request Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove code conditionally compiled for PETSc <3.5
6 participants