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 unused/unsupported physics, provide legacy code last working git tag #359

Closed
bartgol opened this Issue Aug 27, 2018 · 45 comments

Comments

Projects
10 participants
@bartgol
Copy link
Contributor

bartgol commented Aug 27, 2018

This issue is to decide what packages are to be discontinued in Albany. By that I mean packages/physics that are no longer maintained, and that have become a maintenance burden, especially when Albany undergoes refactoring efforts.

Such packages should be removed from the current master (together with all their configurations options/macros that are specific to them). We should provide an Albany tag (together with a working trilinos tag) where such packages are still present and functional (= test passing).

Of course, if some user is willing to port such packages to the current master framework, they are more then welcome. We only do this because of the lack of manpower (and funding money) to maintain packages.

Here is a list of the current packages/physics/subfolders that are non-core. Please, add a comment below if you want the package to remain fully supported in the master branch. By requesting such maintenance, you should also commit some of your time to perform such maintenance. We cannot afford to keep packages that have no manpower to maintain them. For such packages, we can only provide a working git tag. I will update this message adding a checkmark next to the package, and one or more names for people responsible for maintaining it.

  • Aeras, @ikalash
  • AFRL (waiting for a response, probably ok with a tag)
  • AMP (ok with tag)
  • ANISO (ok with tag)
  • ATO
  • CTM (ok with tag)
  • Hydride (ok with tag)
  • LandIce, @bartgol, @mperego
  • LCM, @ikalash, @lxmota (Alejandro, @ikalash is volunteering you)
  • MOR (ok with tag)
  • Peridigm (keep, owner?)
  • QCAD (ok with tag)
  • SCOREC (keep, owner?)
  • Tsunami, @ikalash
  • Dakota (ok with tag)

I would like to get a response by next week, so that we can take a decision at the next Albany conference call.

@bartgol

This comment has been minimized.

Copy link
Contributor Author

bartgol commented Aug 27, 2018

I should add that if a package has no issues or can be maintained with minimal effort, we should try to keep it in master. I am not trying to discontinue all packages that are not working but could with just a few minutes of coding. We are still trying to make an effort to keep Albany a truly multiphysics library. However, there are some efforts ongoing that are already requiring a lot of time, and for which it would take considerable effort to maintain all packages.

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Aug 27, 2018

Should we tag the last known owners of some of the packages to bring this issue to their attention and have them comment?

@bartgol

This comment has been minimized.

Copy link
Contributor Author

bartgol commented Aug 27, 2018

Good idea. Tagging people listed in CODEOWNERS: @gahansen , @tjcorona , @ibaned , @bgranzow , @lxmota , @mperego .

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Aug 27, 2018

Also: @enielse (QCAD), @jrobbin (ATO), @plindsa and @jfike (MOR).

@bartgol

This comment has been minimized.

Copy link
Contributor Author

bartgol commented Aug 27, 2018

@ikalash You may also want to update CODEOWNERS, cause it lists you as owner of those packages.

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Aug 27, 2018

Where is CODEOWNERS? Is it in a file in the repo? I would not consider myself the owner of these packages...

@bartgol

This comment has been minimized.

Copy link
Contributor Author

bartgol commented Aug 27, 2018

Yes, in the top folder. Apparently github parses it, and uses it also to notify code owners when a PR touches paths that are under their responsibility. Makes it easier (actually, automatic) for people without write access to the repo to tag the proper code maintainer.

@bgranzow

This comment has been minimized.

Copy link
Contributor

bgranzow commented Aug 27, 2018

As far as I know, the CTM package has no plans for further use and depends on a version of Simmetrix that I no longer have access to (meaning I can't really maintain it). I would be fine with removing it. It might make sense for me to ask if anyone at RPI cares about this at the next Albany telecon.

ikalash added a commit that referenced this issue Aug 27, 2018

@ibaned

This comment has been minimized.

Copy link
Contributor

ibaned commented Aug 29, 2018

@maxrpi is okay with leaving CTM as a tag and not updating it.

@jrobbin

This comment has been minimized.

Copy link
Contributor

jrobbin commented Aug 30, 2018

I'll commit to maintain the ATO content. There's a project starting in FY19 that will leverage this capability.

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Sep 4, 2018

We should also probably add Peridigm and SCOREC to the list of packages. There have been failing tests for these for awhile (see issue #360).

@bartgol

This comment has been minimized.

Copy link
Contributor Author

bartgol commented Sep 4, 2018

Done!

So far there's a lot of unchecked packages. Since there was no telecon on Labor Day, I think I will wait till the next telecon before we make any final decision.

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Sep 4, 2018

@djlittl : can you please comment on this issue regarding plans for supporting peridigm? @ibaned , @bgranzow : are you guys the owners of SCOREC still? If not, can you please tag the current owner so they can comment on this issue.

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Sep 6, 2018

Another related question: is anyone interested in AlbanyDakota and AlbanyAnalysis? If not perhaps these should be removed too. I am the only one testing these nightly, it looks like.

@lxmota

This comment has been minimized.

Copy link
Contributor

lxmota commented Sep 6, 2018

Not as far as LCM is concerned.

@bartgol

This comment has been minimized.

Copy link
Contributor Author

bartgol commented Sep 6, 2018

I think we need AlbanyAnalysis for optimiztion, no?

@mperego

This comment has been minimized.

Copy link
Contributor

mperego commented Sep 6, 2018

Yes we need AlbanyAnalysis for optimization. It is also used in LCM Peridigm.

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Sep 6, 2018

Oh right. So, for optimization and peridigm you use Dakota via AlbanyAnalysis? Somehow I didn't realize that. I was referring to the examples that have "Analysis" in the name. I upgraded the OS on my machine and now there's some issue with the tests that use Dakota. It may have to do with me having too old of a version of Dakota to work with gcc/8.1.1 which is very new. I thought if the tests aren't needed I can just disable them, hence this question.

@mperego

This comment has been minimized.

Copy link
Contributor

mperego commented Sep 6, 2018

We don't use Dakota for the optimization for LandIce and Peridigm, we use ROL.

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Sep 6, 2018

Ok, this is what I thought. Let me rephrase my question: is the use of Dakota by AlbanyDakota/AlbanyAnalysis obsolete? I suspect the answer is yes, but it would be good to have someone second that. Maybe we can add it to the checklist at the top of this issue.

@bartgol

This comment has been minimized.

Copy link
Contributor Author

bartgol commented Sep 7, 2018

I thought John was using Albany and Dakota for the UQ stuff in Prospect

@mperego

This comment has been minimized.

Copy link
Contributor

mperego commented Sep 7, 2018

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Sep 7, 2018

We never used embedded UQ capabilities in Albany for PISCEES/ProSPect. We used to wrap DAKOTA around Albany, but that was years ago; now we have a different approach for UQ in ProSPect that doesn't use DAKOTA at all.

@bartgol

This comment has been minimized.

Copy link
Contributor Author

bartgol commented Sep 7, 2018

Is there a test in one of the nightly builds that tests Albany-Dakota interfaces? If not, then I also wonder if it's ok to keep it around. It doesn't bother much, since it's basically two files, with little to maintain (or nothing at all if you don't/can't have Dakota on your machine), so it's not the same as maintaining packages in Albany. It is more a question of "can we guarantee it works, if someone tomorrow downloads Albany and uses it with Dakota?".

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Sep 7, 2018

There were a couple of tests that only I was running. I upgraded the OS on my machine and they seg fault now. It may have to do with me using an old version of Dakota. I am wondering if maintaining the tests is worthwhile if no one is using them.

@agsalin

This comment has been minimized.

Copy link
Contributor

agsalin commented Sep 7, 2018

AlbanyDakota was an earlier interface to Dakota. Dakota is also available as an option under AlbanyAnalysis via a Piro interface to Dakota. I had also kept the direct Dakota interface around since it had the cool capability of having Dakota select MPI_Comm for Albany to use, and it could spawn numerous Albany runs in parallel function evaluations on sub-communicators. That's the history/nostalgia -- No reason to keep AlbanyDakota anymore or the two files.

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Sep 7, 2018

Thanks for the info, @agsalin. I'm fine keeping things around even if they're not used provided it's fairly straightforward to do so. I can look at the Dakota issue when I have more time in a few weeks, which would likely result in opening an issue for the Dakota team to look at b/c I don't think the problem is in Albany from the backtrace. I'm not sure if that'll be helpful to the Dakota team or a waste of their time - this is why I started the discussion.

@ibaned

This comment has been minimized.

Copy link
Contributor

ibaned commented Sep 8, 2018

I don't know... I'd think twice about spending time/money maintaining something that isn't used...

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Sep 8, 2018

@ibaned . Right I agree. Let’s talk about it on the telecom Monday. Hopefully we can make some definite decisions about what stays and what goes.

@djlittl

This comment has been minimized.

Copy link
Contributor

djlittl commented Sep 10, 2018

I am using the code for coupling with Peridigm for a paper that I'm working on, and it's also been cited on a few SBIR proposals over the last two years. I honestly don't know what the right path forward is. I have no funds for testing, maintenance, support, etc. It's also tricky to test, because it requires a Peridigm build. Suggestions welcome.

@gahansen

This comment has been minimized.

Copy link
Member

gahansen commented Sep 10, 2018

I see a "strategic" benefit for some of these packages - like Peridigm. While we don't have a path to maintain this link as we would like, it involves two major Sandia capabilities and I hold out hope that interest in multiscale material behavior might make this an important feature "any day now". I guess the question is - is it harder to keep it minimally functional (the current dashboard green) or tag it and bring it back to life when the time comes? Is it possible to solicit a volunteer to keep the dashboard green on the tests of the Peridigm link?

@gahansen

This comment has been minimized.

Copy link
Member

gahansen commented Sep 10, 2018

Tagging @tjcorona - TJ what do you see as the future of the AFRL subdirectory / capability? I moved a little of that into one of the LCM evaluators but I don't think we finished that or included a test for it.

@gahansen

This comment has been minimized.

Copy link
Member

gahansen commented Sep 10, 2018

I am the "owner" of Hydride. It still compiles on CEE but isn't tested now. I'd suggest we tag the commit that removes this and call that good.

@bartgol

This comment has been minimized.

Copy link
Contributor Author

bartgol commented Sep 13, 2018

There are some evaluators/responses in the main PHAL folders that have the QCAD prefix. If QCAD is eliminated, what should we do with them? Should we purge them, or move them to PHAL?

@mperego

This comment has been minimized.

Copy link
Contributor

mperego commented Sep 13, 2018

If they are used by other packages, or if they could be useful, I think it would make sense to move them to PHAL. Otherwise I'd purge them.

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Sep 13, 2018

I agree. I believe there are some capabilities from the QCAD namespace used in LCM (e.g., the MaterialDatabase). @lxmota would be able to say more. I'd move these to the PHAL namespace.

@bartgol

This comment has been minimized.

Copy link
Contributor Author

bartgol commented Sep 13, 2018

Ok, I agree. On a similar note, I should try to keep stuff like Albany_ThermoElectrostaticsProblem.*pp, which is only compiled if QCAD is enabled, but does not seem to use any of the QCAD evaluators/responses. It was probably a good idea to move that pb to the main Albany anyways.

@bartgol

This comment has been minimized.

Copy link
Contributor Author

bartgol commented Sep 13, 2018

Wait, that problem reuires QCAD_PoissonResid. I may grab that single non-phal evaluator, and port it to the pde examples.

@lxmota

This comment has been minimized.

Copy link
Contributor

lxmota commented Sep 13, 2018

@ikalash No, long ago I removed that QCAD dependency from LCM. I created a MaterialDatabase with far fewer features for use in LCM. The QCAD version was overly complex for LCM.

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Sep 13, 2018

@lxmota : ah yes I remember now. Good, it should simplify the removal of QCAD then.

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Sep 17, 2018

Per our discussion this morning, I checked out Trilinos from before STK broke things (8/20), rebuilt it, built Albany on top of it, and ran my nightly test scripts. Here are the packages enabled there:

    "-DALBANY_TRILINOS_DIR:PATH=${TRILINSTALLDIR}"
    "-DENABLE_LCM:BOOL=ON"
    "-DENABLE_CONTACT:BOOL=OFF"
    "-DENABLE_LCM_SPECULATIVE:BOOL=OFF"
    "-DENABLE_HYDRIDE:BOOL=ON"
    "-DENABLE_SG:BOOL=OFF"
    "-DENABLE_ENSEMBLE:BOOL=OFF"
    "-DENABLE_LANDICE:BOOL=ON"
    "-DENABLE_TSUNAMI:BOOL=ON"
    "-DENABLE_AERAS:BOOL=ON"
    "-DENABLE_QCAD:BOOL=ON"
    "-DENABLE_MOR:BOOL=ON"
    "-DENABLE_ATO:BOOL=ON"
    "-DENABLE_ALBANY_EPETRA_EXE:BOOL=ON"
    "-DENABLE_AMP:BOOL=OFF"
    "-DENABLE_ASCR:BOOL=OFF"
    "-DENABLE_AFRL:BOOL=OFF"
    "-DENABLE_CHECK_FPE:BOOL=OFF"
    "-DENABLE_MPAS_INTERFACE:BOOL=OFF"
    "-DENABLE_CISM_INTERFACE:BOOL=ON"
    "-DENABLE_CISM_CHECK_COMPARISONS:BOOL=ON"
    "-DENABLE_CISM_EPETRA:BOOL=ON"
    "-DENABLE_CISM_REDUCED_COMM:BOOL=OFF"
    "-DSEACAS_EPU=/home/ikalash/Trilinos/seacas-build/install/bin/epu"
    "-DSEACAS_EXODIFF=/home/ikalash/Trilinos/seacas-build/install/bin/exodiff"
    "-DSEACAS_ALGEBRA=/home/ikalash/Trilinos/seacas-build/install/bin/algebra"
    "-DCISM_INCLUDE_DIR:FILEPATH=${CTEST_SOURCE_DIRECTORY}/cism-piscees/libdycore"
    "-DINSTALL_ALBANY:BOOL=ON"
    "-DCMAKE_INSTALL_PREFIX:BOOL=${CTEST_BINARY_DIRECTORY}/IKTAlbanyInstall"
    "-DENABLE_PARAMETERS_DEPEND_ON_SOLUTION:BOOL=ON"
    "-DCISM_EXE_DIR:FILEPATH=${CTEST_BINARY_DIRECTORY}/IKTCismAlbany"
    "-DENABLE_USE_CISM_FLOW_PARAMETERS:BOOL=ON"
    "-DENABLE_LAME:BOOL=OFF"

When I run ctest in a regular full build (with the above settings), unfortunately there are some LCM tests failing, namely the dynamics Schwarz tests I mentioned seem to have broken recently:

        307 - Schwarz_Alternating_Dynamics_Cubes (Failed)
	308 - Schwarz_Alternating_Dynamics_CubesInelastic (Failed)
	309 - Schwarz_Alternating_Dynamics_CubesInelastic_Parallel (Failed)
	310 - Schwarz_Alternating_Dynamics_Clamped_Parallel (Failed)
	311 - Schwarz_Alternating_Dynamics_Torsion_Parallel (Failed)

The error looks like this: http://cdash.sandia.gov/CDash-2-3-0/testDetails.php?test=3938397&build=75966 . It looks like this may have been a problem introduced by a merge of a PR by @bartgol looking at past github issues, namely #350. These were just showing up on proxima, but now they show up on my machine. I bet that's because I upgraded my OS to Fedora 28 and gcc/8.1.1, which are now consistent with proxima.

The question is do we want to wait till these tests are fixed before moving things to a tag or not? If one does not build Trilinos with Tempus turned on, the tests will not run (and hence will not fail).

There are also 4 failing tests in an OpenMP build:

       141 - MechanicsTensileCT (Failed)
        230 - CrystalPlasticity_OrientationOnMesh (Failed)
        231 - CrystalPlasticity_OrientationOnMesh_np4 (Failed)
        243 - CrystalPlasticity_SchwarzBar_modified_np1 (Failed)

This seems to be another existing known issue, namely #339. As I said this morning, I would not be too concerned about the OpenMP tests since I don't think anyone using the obsolete physics moved to a tag will be running with OpenMP (nor should they, since most of the physics are Epetra-based).

Here is the Trilinos hash that produced the results described above: 22c79ca9434ce101fc4c4200a90c5d9e42692ca5 . BTW, this is develop Trilinos. I am not sure if you were planning to give SHAs for develop or master Trilinos @bartgol . I think it doesn't matter as long as things are clean with the SHA.

@ibaned

This comment has been minimized.

Copy link
Contributor

ibaned commented Sep 17, 2018

I think we agreed that since LCM will continue to be supported, we don't need LCM's tests to pass in order to approve tags for other sub-projects, as long as all of their sub-project tests pass.

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Sep 17, 2018

One thing that came to mind while writing the reply above. Maybe we should mention the compilers used to create the working builds in the READMEs? It can happen that things start failing with newer compilers (as evidenced by the LCM Schwarz tests). So it's conceivable that something might break with time as compilers are upgraded even though the tag was working perfectly at the time it was created.

@bartgol bartgol added this to Ready in Albany Oct 1, 2018

@bartgol

This comment has been minimized.

Copy link
Contributor Author

bartgol commented Oct 3, 2018

This was completed by PR #363 .

@ikalash I did not specify working compilers in the readme's or in the tags. I will edit them, by looking at the compilers currently used on cdash.

@bartgol bartgol closed this Oct 3, 2018

Albany automation moved this from Ready to Done Oct 3, 2018

@ikalash

This comment has been minimized.

Copy link
Contributor

ikalash commented Oct 3, 2018

@bartgol sounds good. The compiler I use is: gcc/8.1.1 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment