search

DOI-USGS / ISIS3

Integrated Software for Imagers and Spectrometers v3. ISIS3 is a digital image processing software package to manipulate imagery collected by current and past NASA and International planetary missions.
https://isis.astrogeology.usgs.gov
Other
192 stars 166 forks source link

USGSCSM Missing from LTS Installation #5382

Open jlaura opened 7 months ago

jlaura commented 7 months ago

ISIS version(s) affected: 8.0.0+

Description

ISIS is not shipping with USGSCSM (libusgscsm). It is not specified in the environment.yml or meta.yml. The CSM library is. Please add the USGSCSM as a dependency.

How to reproduce

conda list usgscsm

Possible Solution

Update environment.yml and meta.yaml to include the USGSCSM.

Additional context

acpaquette commented 7 months ago

I hesitate to include USGSCSM as a dependency for ISIS, I agree that we need to make it so when you install USGSCSM into your environment ISIS can find the plugin as you mention in #5383. However, the CsmCamera in ISIS should be able to work with other CSM plugins but we likely won't at those as dependencies either.

I also think that we could add/update documentation for using CSM as you mentioned in #5384.

jlaura commented 7 months ago

I hesitate to include USGSCSM as a dependency for ISIS

Can you please expand on why?

acpaquette commented 7 months ago

The library was designed to be a plugin. You should be able to install various versions without being dependent on the host program, in this case ISIS. This also means you should be able to install other CSM camera models without needing them as dependencies in ISIS. The biggest gain from not having USGSCSM as a dependency is to give users flexibility to install different versions of the software without needing to install a completely different version of ISIS.

There may be some merit for including some default version of USGSCSM in ISIS so that the CSMCamera works out of the box by installing ISIS, but a user could also just add usgscsm to there env creation and get a similar result.

The biggest problem USGSCSM has right now is that it isn't self contained and ships with dependencies that aren't baked into the plugin, basically making it not a plugin. The biggest of which is proj but wasn't the first USGSCSM dependency to not be built into the plugin. This goes back to the work being done on USGSCSM to actually make it a true plugin and build in all of it's own dependencies. So with that noted we should either finishing making USGSCSM a true self contained plugin or add it as a direct dependency which we likely shouldn't.

What other reasons are there to make USGSCSM a direct dependency so that I know for context?

jlaura commented 7 months ago

More questions and thoughts about this approach:

What other reasons are there to make USGSCSM a direct dependency so that I know for context?

  1. We are promoting ISIS as fully supporting USGSCSM (see above). If we aren't even shipping USGSCSM with ISIS then the path of least resistance (most likely to be taken be users) is to use an ISIS sensor model.
  2. If ISIS isn't shipping USGSCSM, who are we expecting to use it? At a meta-level, what was the point in going down the CSM road if it is not intended to be the default sensor model for ISIS?
Kelvinrr commented 7 months ago

I agree with @jlaura in that I think it makes the most sense to include the plugin. Considering its USGSCSM is developed by us and is not third-party, including some version as part of the package makes sense and seems like a very low friction, high value change.

With that said:

Sure, usgscsm is designed to be a plugin. Aren't lots of piece of ISIS designed that way? If so, what other pieces need to be shipped this way and does that start to become a significant burden on a non-expert user?

A lot of pieces of ISIS are designed as plugin but they are also part of the same library, so this comparison between ISIS plugins and the USGSCSM plugin doesn't make sense. If anything, it points out that we probably should be shipping those plugins separately. One could easily argue that we should make ISIS more a la carte (something we have been moving towards). But I don't think we should waste time on these rabbit holes here. It's more about what is easy and makes sense to do right now given ISIS's current paradigms.

Yes, in theory, a user being able to install different versions of USGSCSM is a plus. In what general instances do you see a user actually doing that? What most of the time use cases would I want to be swapping between, say v1.6.0 and v1.7.0? Outside comparison of changes (which one wouldn't likely do in ISIS anyway) what is the use case that meets the needs of the majority of users?

Same as above, I think this is more just rhetorical white noise. We either include it or we don't, either way a user can change the version and there are often niche reasons for this. We have also had many discussions very explicitly pushing towards enabling users to download portions of functionality.

"a user could also just add usgscsm to there env creation and get a similar result" - I feel like I am very in the know and I clearly did not know what to do. Is this a documentation issue, sure, but it isn't like USGSCSM is an ISIS data area download. It's a dynamically linked library. Let's assume for a minute we have great docs. Do we also have testing in place to ensure that whatever version of usgscsm I throw at ISIS, it is installable and works?

I think we can all agree that adding another layer of complexity increases the risk of error. Users have had issues with the additional ISISDATA step for as long as I've known of the software. Your self-assessment on how competent you may or may not be is irrelevant, it's a matter of an added layer of complexity. Added complexity we will naturally adopt as ISIS becomes more modular regardless. Additionally, if we include it in the lib or not, we also still need to maintain some range of USGSCSM versions that are supported by ISIS and document all that at some point.

I still believe that inclusion of proj inside usgscsm is a bad idea. The reasons not to ship it with ISIS are another point reinforcing that opinion.

You, Adam and I were all in a meeting where we all very explicitly agreed to go ahead with using proj. It's not even relevant here since ISIS already has a proj dependency.

Right now, it seems that usgscsm is being very much treated as an afterthought inside ISIS. That's fine, assuming that it is presented as such (which it is not). We are actively discussing creating CSM sensor models with mission teams, are working with CSM sensor models in production, and I thought, are working to have feature complete support for CSM inside ISIS. If that isn't the case, that needs to be actively discussed.

Again, I don't see the necessity in wording things in this way. The second sentence says it's fine that USGSCSM is presented as an afterthought while the rest contradicts this and implying that it would be a problem if it were an afterthought. Considering you led this team for a long time through most of USGSCSM's development, I find your confusion on this topic very bizarre. I think we can all agree that USGSCSM is important, but conversations should focus more on particulars, like whether or not it makes sense to put in USGSCSM right now. The point of this issue.

If ISIS isn't shipping USGSCSM, who are we expecting to use it? At a meta-level, what was the point in going down the CSM road if it is not intended to be the default sensor model for ISIS?

Implying that Adam doesn't expect people to use it seems disrespectful and a blatant misrepresentation of his original point. This is more persuasive if you just remove the first sentence.

I find it somewhat disturbing that a simple request can escalate into what feels like predominately rhetorical nonsense that isn't productive. I think we should avoid such rabbit holes in the future discussing specific issues and maybe bring it up these higher-level concerns at an ISIS TC meeting. It would have been easier to make a persuasive argument to include USGSCSM into the main ISIS package without all the unwarranted aggression.

Kelvinrr commented 7 months ago

My opinion on why we should include USGSCSM:

  1. It's a low friction to implement and maintain the dependency.
  2. Makes it easier for users
  3. Sets the expectation that USGSCSM is a core part of the library.

Shipping ISIS a la carte I think makes sense, as we have moving towards that goal, but that is a higher-level discussion outside the scope of this issue.

jlaura commented 7 months ago

@Kelvinrr I appreciate your second comment. The first looks like a hard misread to the contribution I am trying to make. There is no intentional aggression or confusion on my side. Your opinions, picking apart my text are not constructive for the issue. Send me an email if you want to discuss this thread further.

oleg-alexandrov commented 6 months ago

I agree that the current state of things is not the best. The csminit program does not work. Not even after usgscsm is installed in the same conda env. No message is given as to what is failing. No doc exists.

The usgscsm library should be a dependency of ISIS, and documentation should be added how to look up plugins.

github-actions[bot] commented 3 weeks ago

Thank you for your contribution!

Unfortunately, this issue hasn't received much attention lately, so it is labeled as 'stale.'

If no additional action is taken, this issue will be automatically closed in 180 days.

If you want to participate in our support prioritization meetings or be notified when support sprints are happening, you can sign up the support sprint notification emails here.

Read more about our support processs here

jlaura commented 3 weeks ago

If this is not fixed, it absolutely needs to be considered during the next support sprint. @chkim-usgs

acpaquette commented 4 days ago

More info here, USGSCSM 2.0.1 works on Mac as intended. Meaning, you can install and load it without issues in the ISIS csminit process.

While working on the ISIS 8.2 RC2 release, I tried using csminit on a linux box with USGSCSM 2.0.1 and got a segfault but the CSM state was written to the cube. After some initial testing the error seemed to be in USGSCSM and not ISIS but further debugging is required.

Given that USGSCSM 2.0.1 is broken on linux boxes, we will work to get that fixed before making it a dependency in ISIS