trilinos: add adelus, aprepro and teuchos variants #28935

prwolfe · 2022-02-14T21:05:10Z

Gemma uses a different stack than most FE codes. All new
variants are off by default and this shoudl not affect
current users.

sethrj

Hi @prwolfe , thanks for your contribution. I've made an effort over the last year or so to trim down the number of variants in Trilinos, because each new one requires a full set of conflicts to be duplicated from the Trilinos build logic (TriBITS) in order to gauge which combinations of variants have a hope of working. For example, I removed teuchos because it's a prerequisite to almost everything in trilinos: under what circumstances do you turn it off? I have the same question about triutils: what is the benefit of being able to manually enable or disable it? Without the variant, the trilinos build logic will toggle it on or off as needed.

The adelus variant needs to have conflicts to inform spack what dependencies it has.

Finally, to reduce the combinatorial horror that is the trilinos package option system, there's been a move toward using the external seacas package rather than the one annexed by Trilinos through the Tribits build system. Does it makes sense for your application to use that package instead of the Trilinos-vendored version?

prwolfe · 2022-02-15T14:51:58Z

@sethrj - Trimming the number of variants in Trilinos may be a lost cause in the long run ;->. I agree that the current package is unwieldy at best and the conflicts section is the only current defense. I have been talking with @jwillenbring about adding more generality to this file (which makes reduction difficult) and I see the need for serious unit testing for this package to enable that. Adelus does not appear to drive or conflict with any other packages. I added Teuchos because Gemma has it explicitly turned on, but I cannot say I have seen a build with it off. Can you think of one @jwillenbring?

As for Seacas, I do prefer using it from Trilinos in order to reduce the number of repos needed for most builds. I am willing to change that if needed, but the Trilinos team will need to decide to not support a distributed package (not something I would recommend.)

A lot of what's causing confusion to my mind is that TriBits and spack are doing a lot of the same (and potentially conflicting) configuration work. I would recommend consolidating that where we can over time. I will remove Teuchos and re-submit if it works.

Thanks!

Paul

sethrj · 2022-02-15T15:29:57Z

Trimming the number of variants in Trilinos may be a lost cause in the long run ;->. I agree that the current package is unwieldy at best and the conflicts section is the only current defense. I have been talking with @jwillenbring about adding more generality to this file (which makes reduction difficult) and I see the need for serious unit testing for this package to enable that. Adelus does not appear to drive or conflict with any other packages. I added Teuchos because Gemma has it explicitly turned on, but I cannot say I have seen a build with it off. Can you think of one @jwillenbring?

Since Gemma requires it on and spack always turns it on, I think we should be OK :)

As for Seacas, I do prefer using it from Trilinos in order to reduce the number of repos needed for most builds. I am willing to change that if needed, but the Trilinos team will need to decide to not support a distributed package (not something I would recommend.)

A lot of what's causing confusion to my mind is that TriBits and spack are doing a lot of the same (and potentially conflicting) configuration work. I would recommend consolidating that where we can over time.

Indeed, these two issues are intertwined. TriBITS is a framework for a "megarepo"-type build system where you always build everything at the same time. That works well if all your code is "owned" by a single entity (e.g. if you're Microsoft, Google, or 2005-era Sandia) but becomes problematic in an open source ecosystem, which is where the scientific and HPC communities have been trending for the last 10 years. Spack is a package manager that tries to handle software dependencies from a variety of sources; and TriBITS has the philosophy that everything should be built inside its single build system. The two philosophies are largely incompatible... and the TriBITS philosophy also makes agile development more difficult. If you incorporated Gemma into TriBITS you'd have to rebuild your trilinos software stack every time you change a CMake configure variable.

My understanding is that SEACAS is trying to separate from Trilinos just like Kokkos is now providing a separate package. @gsjaardema is that accurate?

If you are using Spack to set up dependencies for Gemma then changing from trilinos to trilinos&seacas won't be much of a stretch. But I understand wanting to keep dependencies simple...

gsjaardema · 2022-02-16T13:23:37Z

@sethrj
SEACAS has always had a separate repository and does snapshots into Trilinos (and another internal repository) periodically. I'm not sure what the long-term goal is -- continue to be provide an internal copy of SEACAS in Trilinos or have Trilinos use an external SEACAS.

sethrj · 2022-02-16T13:29:33Z

Since Kokkos is moving toward external, and @keitat can confirm that Trilinos is trying to be more agile about external dependencies (and being a better software ecosystem player), I think external SEACAS is the direction we want to head. Looking at the Trilinos package dependency chain, it looks like there's perhaps one or two connections into SEACAS from the main trilinos, and I think no dependencies of SEACAS on trilinos? I strongly suggest we move toward simplifying the Trilinos spack package and the larger software ecosystem by removing SEACAS from Trilinos in the long term.

packages-trilinos-noclusters.dot.pdf

prwolfe · 2022-02-16T14:36:03Z

Seems like a decision for the trilinos framework team (@jwillenbring) and @gsjaardema to me.

sethrj · 2022-02-16T21:34:05Z

In the meantime @prwolfe if you remove the teuchos variant and add the required conflicts for the new packages (denoting tribits package requirements) I'll approve even with the new SeaCas package.

sethrj · 2022-02-16T21:34:47Z

Perhaps @bartlettroscoe has some history on the incorporation of SEACAS into Trilinos?

prwolfe · 2022-02-16T21:52:17Z

Will do – likely early tomorrow morning. From: "Seth R. Johnson" ***@***.***> Reply-To: spack/spack ***@***.***> Date: Wednesday, February 16, 2022 at 2:34 PM To: spack/spack ***@***.***> Cc: "Wolfenbarger, Paul R" ***@***.***>, Mention ***@***.***> Subject: [EXTERNAL] Re: [spack/spack] trilinos: add adelus, aprepro and teuchos variants (PR #28935) In the meantime @prwolfe<https://github.com/prwolfe> if you remove the teuchos variant and add the required conflicts for the new packages (denoting tribits package requirements) I'll approve even with the new SeaCas package. — Reply to this email directly, view it on GitHub<#28935 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AB2OSCJD7VFKJPICU3XQWCLU3QJ5VANCNFSM5OMSUCVA>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

bartlettroscoe · 2022-02-16T22:01:39Z

Perhaps @bartlettroscoe has some history on the incorporation of SEACAS into Trilinos?

That question is above my pay-grade (and I had no part in that work).

My understanding is that SEACAS is trying to separate from Trilinos just like Kokkos is now providing a separate package. @gsjaardema is that accurate?

The plan this FY is to allow SEACAS to be built and installed as an independent CMake project (i.e. as a Spack seacas package) and then build the remaining Trilinos packages against that install. Same goes for Kokkos and KokkosKernels (and . See TriBITSPub/TriBITS#367.

TriBITS has the philosophy that everything should be built inside its single build system.

That is currently true but we are working to allow TriBITS to build packages in any arbitrary collection of smaller CMake projects that you want. That way, you can build (and install) all at once or in smaller subsets (whatever makes the most sense for the use case you are working). That is the best you can get as it leads to very fast iterative development cycles using just the software you are editing. The basics of that will be complete with TriBITSPub/TriBITS#367. The ultimate goal of this work is to allow any arbitrary TriBITS package to be built and installed as its own CMake project (by finding its pre-built/pre-installed upstream dependencies with find_package(...)).

bartlettroscoe · 2022-02-16T22:16:40Z

A lot of what's causing confusion to my mind is that TriBits and spack are doing a lot of the same (and potentially conflicting) configuration work. I would recommend consolidating that where we can over time.

Yes, the Spack Trilinos/packages.py file should be updated to use TriBITS logic for addressing inner Trilinos dependencies and exposing variants for enables and disables of individual Trilinos packages. All you need is a usable cmake and python to do that. You don't need to actually configure any CMake packages. (See Reduced Package Dependency Processing and tribits_adjust_package_enables().)

For example, I removed teuchos because it's a prerequisite to almost everything in trilinos: under what circumstances do you turn it off?

I agree with that. You really can't build anything useful in Trilinos without building Teuchos. But an automated system that used TriBITS logic could run the enables/disables and provide feedback about conflicts before having to actually configure Trilinos.

But any layer you put around the Trilinos CMake build is going to be inadequate for some customers because it will not expose all of the CMake cache vars (i.e. knobs) that are available. I was successful with ATDM (which included GEMMA) by exposing fairly small number of extra "knobs" as documented in:

https://github.com/trilinos/Trilinos/blob/develop/cmake/std/atdm/README.md#build-name-keywords

bartlettroscoe · 2022-02-16T22:40:28Z

var/spack/repos/builtin/packages/trilinos/package.py

+        if '+aprepro' in spec:
+            options.extend([
+                define_trilinos_enable('SEACAS', True),
+                define_trilinos_enable('SEACASAprepro', True),
+            ])
+        else:
+            options.extend([
+                define_trilinos_enable('SEACASAprepro', False),
+            ])


This type of logic should not exist in this package.py file. This the type of logic that TriBITS handles automatically.

The SEACASAprepro options need to be there so that the variants are consistent with what's actually installed... the define_trilinos_enable('SEACAS', True) could probably be dropped though. I think this one (and several of the similar ones) can be written:

Suggested change

if '+aprepro' in spec:

options.extend([

define_trilinos_enable('SEACAS', True),

define_trilinos_enable('SEACASAprepro', True),

])

else:

options.extend([

define_trilinos_enable('SEACASAprepro', False),

])

options.append(define_trilinos_enable('SEACAS', 'aprepro'))

The issue with variants is that they have to be determined at concretization time in Spack to ensure they're compatible with all the other software in a spec (or set of specs in an environment). We can't just defer to tribits logic. The closest we could possibly do would be to parse the tribits dependencies for all the different versions of trilinos we want to support, then convert those into the appropriate variants and conflicts...

The issue with variants is that they have to be determined at concretization time in Spack to ensure they're compatible with all the other software in a spec (or set of specs in an environment).

I see, that makes sense.

The closest we could possibly do would be to parse the tribits dependencies for all the different versions of trilinos we want to support, then convert those into the appropriate variants and conflicts...

Yes, that is what should be done. Given the generated file XML file TrilinosDependencies.xml (which could just be returned as a string to STDOUT as called inside Trilinos/package.py), Python code in Trilinos/package.py could process this and automatically generate the package enable/disable variants and provide the conflicts list (which will take some graph algorithms to give Spack what it wants and may need to be implemented carefully to be performant).

jwillenbring · 2022-02-22T20:19:55Z

The only situations I can think of where someone might want to disable Teuchos are ones where the package the person wants to use is likely a good candidate to be split off into a separate Spack package, like Kokkos and SEACAS.

Remove variant for secas executable "aprepro". It will always build now Remove variant for "teuchos". This is need for virtually all builds Confirm that adelus and triutils are without conflicts in either direction

sethrj · 2022-02-25T00:16:16Z

var/spack/repos/builtin/packages/trilinos/package.py

@@ -126,6 +127,7 @@ class Trilinos(CMakePackage, CudaPackage, ROCmPackage):
    variant('tempus',       default=False, description='Compile with Tempus')
    variant('tpetra',       default=True, description='Compile with Tpetra')
    variant('trilinoscouplings', default=False, description='Compile with TrilinosCouplings')
+    variant('triutils',     default=False, description='Compile with Triutils')


As my original review requested, these new variants need conflicts statements to attempt to mirror the TriBITS package logic in Spack (since all valid combinations of variants should produce a working binary, so that different libraries and apps can guarantee that they'll work together). Look at amesos2 as an example.

I still don't understand what the extra triutils variant adds to the package: does your code need it downstream? If so let's just turn it on by default if it doesn't require any upstream dependencies.

sethrj · 2022-03-10T18:54:35Z

var/spack/repos/builtin/packages/trilinos/package.py

@@ -539,6 +538,11 @@ def define_enable(suffix, value=None):
            define_from_variant('Amesos2_ENABLE_LAPACK', 'amesos2'),
        ])

+        if '+aztec' in spec and '+triutils' in spec:


The correct way to handle this is to add a conflict saying "you cannot have aztec enabled and triutils disabled":

conflicts('+aztec', when='~triutils')

Otherwise users who want aztec will find it silently missing!

prwolfe · 2022-03-14T19:24:47Z

Adding a conflict will turn off all of aztec rather than just AztecOO. Seems ok to me, but may not match expectations.

sethrj · 2022-03-15T11:28:14Z

Adding a conflict will turn off all of aztec rather than just AztecOO. Seems ok to me, but may not match expectations

Am I missing something? There is no "Aztec" trilinos package other than AztecOO; the spack option is named aztec for simplicity, and what doesn't match expectations if you disable a package and its dependencies are disabled?

This question may be moot because looking at the actual tribits dependencies, Triutils is a test-only dependency of AztecOO: we're not building tests, so the conflict shouldn't be there. It seems to be a leaf in the dependency tree.

However, adelus requires both kokkos and kokkoskernels, so you will need to update the code to have:

    with when('~kokkos'):
        conflicts('+adelos')
...

kuberry · 2022-03-30T21:29:10Z

aztecoo requires triutils for tests, but not for the library itself.

https://github.com/trilinos/Trilinos/blob/master/packages/aztecoo/cmake/Dependencies.cmake#L3

Question/comment to @sethrj : It seems like we don't have tests as a variant in this recipe, so we can't add +tests to the when portion of the aztecoo conflict.

Is tests a variant we would want to expose? The only concern I have is that it will cause even more conflicts to be registered between various internal Trilinos packages.

Perhaps we should have this conversation elsewhere, but I support the idea of not trying to replicate Tribits dependencies within Trilinos in Spack, and only using Spack to handle conflicts with missing TPLs. Then, if the build fails because of a conflicting combination of packages in Trilinos, then let the user read the error message and turn on/off the appropriate package combinations.

sethrj · 2022-03-30T23:08:40Z

Question/comment to @sethrj : It seems like we don't have tests as a variant in this recipe, so we can't add +tests to the when portion of the aztecoo conflict.

Correct. However, there's a (not very stable or well used?) feature to build a spack package's test at install time, and the package has an attribute run_tests (I think?) which is used in some recipes to change the configuration.

Is tests a variant we would want to expose? The only concern I have is that it will cause even more conflicts to be registered between various internal Trilinos packages.

No: besides the proliferation of options, +test doesn't (or shouldn't) change the installed packages.

Perhaps we should have this conversation elsewhere, but I support the idea of not trying to replicate Tribits dependencies within Trilinos in Spack, and only using Spack to handle conflicts with missing TPLs. Then, if the build fails because of a conflicting combination of packages in Trilinos, then let the user read the error message and turn on/off the appropriate package combinations.

The ideal spack package will never concretize to a configuration that won't build: we always want to prevent an non-installable trilinos. Imagine the use case of building a bunch of dependencies in an environment only to find out your build fails: not ideal. The only problem with this philosophy is clingo's variant conflict error messages, which at present frankly are garbage.

prwolfe · 2022-05-09T14:34:26Z

I think all the comments have been addressed here. Is there a mod that we need to make on this?

sethrj · 2022-05-10T01:33:40Z

var/spack/repos/builtin/packages/trilinos/package.py

    conflicts('~thyra', when='+stratimikos')
    conflicts('+aztec', when='~fortran')
+    conflicts('+aztec', when='~triutils')


Aztec doesn't require triutils. Only its tests do, and we aren't building them.

sethrj · 2022-05-10T01:36:25Z

var/spack/repos/builtin/packages/trilinos/package.py

@@ -126,6 +127,7 @@ class Trilinos(CMakePackage, CudaPackage, ROCmPackage):
    variant('tempus',       default=False, description='Compile with Tempus')
    variant('tpetra',       default=True, description='Compile with Tpetra')
    variant('trilinoscouplings', default=False, description='Compile with TrilinosCouplings')
+    variant('triutils',     default=False, description='Compile with Triutils')


I still don't understand what the extra triutils variant adds to the package: does your code need it downstream? If so let's just turn it on by default if it doesn't require any upstream dependencies.

prwolfe · 2022-05-17T20:18:00Z

@sethrj - triutils is in fact required for this code. I can turn it on by default and most code will never notice as it very small. Are you saying you done want to add a variant? Something like

        define_trilinos_enable('triutils', True),

instead?

sethrj · 2022-05-18T12:06:00Z

@sethrj - triutils is in fact required for this code. I can turn it on by default and most code will never notice as it very small. Are you saying you don't want to add a variant? Something like
        define_trilinos_enable('triutils', True),
instead?

Yep, exactly! Thanks!

kuberry · 2022-05-23T13:59:27Z

Hi all, rereading the comments here and trying to get a sense for what was said:

So @prwolfe, you would like to add triutils as a variant that is off by default, and want it to be turned on whenever aztec is enabled?

@sethrj, you don't want a triutils variant to be added to the package. Were you in favor of the define_trilinos_enable('triutils', True),?

Since the dependencies for aztec are:

SET(LIB_REQUIRED_DEP_PACKAGES Epetra)
SET(LIB_OPTIONAL_DEP_PACKAGES Teuchos)
SET(TEST_REQUIRED_DEP_PACKAGES Triutils)

where Triutils are needed for tests but not for building normally, can we add the variant triutils, default False, and then when @prwolfe wants to build aztec, he specifies +aztec in the spec (removing the conflict for ~triutils when +aztec)?

prwolfe · 2022-05-23T14:23:26Z

Hi all, rereading the comments here and trying to get a sense for what was said:

So @prwolfe, you would like to add triutils as a variant that is off by default, and want it to be turned on whenever aztec is enabled?

@sethrj, you don't want a triutils variant to be added to the package. Were you in favor of the define_trilinos_enable('triutils', True),?

Since the dependencies for aztec are:
SET(LIB_REQUIRED_DEP_PACKAGES Epetra)
SET(LIB_OPTIONAL_DEP_PACKAGES Teuchos)
SET(TEST_REQUIRED_DEP_PACKAGES Triutils)
where Triutils are needed for tests but not for building normally, can we add the variant triutils, default False, and then when @prwolfe wants to build aztec, he specifies +aztec in the spec (removing the conflict for ~triutils when +aztec)?

I am actually ok with always building triutils. It's 32 source and 8 header files, so adds little to the build time. In the long run we need more coordination with whatever we have in the Trilinos system, but that is some time away and I am good with this for the foreseeable future. I set it all to running over the weekend and will try to submit the changes today, but I am involved in the ASC S3C conference and that is taking most of my time. Sorry about that.

Paul

Gemma uses a different stack than most FE codes. All new variants are off by default and this shoudl not affect current users.

Remove variant for secas executable "aprepro". It will always build now Remove variant for "teuchos". This is need for virtually all builds Confirm that adelus and triutils are without conflicts in either direction

There are no dependencies between adelus and any other package, and this is the the only required dependency I could find for triutils other than a test requirement (and this package does not enable tests.)

Basicaklly uses a conflict statement to remove aztec when triutils is not present.

Per the github discussion this will simply be on by default.

prwolfe · 2022-05-23T16:25:30Z

All,

My apologies but I had to rebase onto develop due to a problem I caused in git (basically the old merge of develop into the branch was incorrectly done.) That caused my hand testing to fail during cling bootstrapping which of course is not acceptable. I believe this matches everyone's requests.

Paul

sethrj

Quite satisfactory, thanks for working with us on this.

sethrj

Quite satisfactory! Thanks for working with us on this.

spackbot-app bot added new-variant update-package labels Feb 14, 2022

spackbot-app bot requested review from keitat, kuberry and sethrj February 14, 2022 21:05

sethrj requested changes Feb 14, 2022

View reviewed changes

sethrj changed the title ~~This adds adelus, aprepro and teuchos variants to trilinos package~~ trilinos: add adelus, aprepro and teuchos variants Feb 14, 2022

alalazo assigned sethrj Feb 16, 2022

bartlettroscoe reviewed Feb 16, 2022

View reviewed changes

sethrj requested changes Feb 25, 2022

View reviewed changes

sethrj reviewed Mar 10, 2022

View reviewed changes

spackbot-app bot added the conflicts label Mar 14, 2022

sethrj requested changes May 10, 2022

View reviewed changes

prwolfe added 6 commits May 23, 2022 10:22

This adds adelus, aprepro and teuchos variants to trilinos package

0cc9405

Gemma uses a different stack than most FE codes. All new variants are off by default and this shoudl not affect current users.

Respond to coments in PR spack#28935

a53e389

Remove variant for secas executable "aprepro". It will always build now Remove variant for "teuchos". This is need for virtually all builds Confirm that adelus and triutils are without conflicts in either direction

Add dependency between triutils and aztecoo

96a4bcb

There are no dependencies between adelus and any other package, and this is the the only required dependency I could find for triutils other than a test requirement (and this package does not enable tests.)

Respond to github feedback.

2e27e10

Basicaklly uses a conflict statement to remove aztec when triutils is not present.

Add conflict between ~kokkos and +adelus

b5fcb99

Remove triutils variant per discussion.

810cbf8

Per the github discussion this will simply be on by default.

prwolfe force-pushed the addGemmaRequirementsToTrilinos branch from d32e973 to 810cbf8 Compare May 23, 2022 16:22

sethrj approved these changes May 24, 2022

View reviewed changes

sethrj merged commit 3df4a32 into spack:develop May 24, 2022

iarspider pushed a commit to iarspider/spack that referenced this pull request May 30, 2022

trilinos: add adelus, aprepro and teuchos variants (spack#28935)

f5aad1a

bhatiaharsh pushed a commit to bhatiaharsh/spack that referenced this pull request Aug 8, 2022

trilinos: add adelus, aprepro and teuchos variants (spack#28935)

35d38a9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trilinos: add adelus, aprepro and teuchos variants #28935

trilinos: add adelus, aprepro and teuchos variants #28935

prwolfe commented Feb 14, 2022

sethrj left a comment

prwolfe commented Feb 15, 2022

sethrj commented Feb 15, 2022

gsjaardema commented Feb 16, 2022

sethrj commented Feb 16, 2022

prwolfe commented Feb 16, 2022

sethrj commented Feb 16, 2022

sethrj commented Feb 16, 2022

prwolfe commented Feb 16, 2022 via email

bartlettroscoe commented Feb 16, 2022

bartlettroscoe commented Feb 16, 2022

bartlettroscoe Feb 16, 2022

sethrj Feb 16, 2022

sethrj Feb 16, 2022

bartlettroscoe Feb 16, 2022

jwillenbring commented Feb 22, 2022

sethrj Feb 25, 2022

sethrj May 10, 2022

sethrj Mar 10, 2022

prwolfe commented Mar 14, 2022

sethrj commented Mar 15, 2022

kuberry commented Mar 30, 2022

sethrj commented Mar 30, 2022

prwolfe commented May 9, 2022

sethrj May 10, 2022

sethrj May 10, 2022

prwolfe commented May 17, 2022

sethrj commented May 18, 2022

kuberry commented May 23, 2022

prwolfe commented May 23, 2022

prwolfe commented May 23, 2022

sethrj left a comment

sethrj left a comment

trilinos: add adelus, aprepro and teuchos variants #28935

trilinos: add adelus, aprepro and teuchos variants #28935

Conversation

prwolfe commented Feb 14, 2022

sethrj left a comment

Choose a reason for hiding this comment

prwolfe commented Feb 15, 2022

sethrj commented Feb 15, 2022

gsjaardema commented Feb 16, 2022

sethrj commented Feb 16, 2022

prwolfe commented Feb 16, 2022

sethrj commented Feb 16, 2022

sethrj commented Feb 16, 2022

prwolfe commented Feb 16, 2022 via email

bartlettroscoe commented Feb 16, 2022

bartlettroscoe commented Feb 16, 2022

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

jwillenbring commented Feb 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prwolfe commented Mar 14, 2022

sethrj commented Mar 15, 2022

kuberry commented Mar 30, 2022

sethrj commented Mar 30, 2022

prwolfe commented May 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prwolfe commented May 17, 2022

sethrj commented May 18, 2022

kuberry commented May 23, 2022

prwolfe commented May 23, 2022

prwolfe commented May 23, 2022

sethrj left a comment

Choose a reason for hiding this comment

sethrj left a comment

Choose a reason for hiding this comment