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

USGSCSM Missing from LTS Installation #5382

Open
jlaura opened this issue Dec 20, 2023 · 14 comments
Open

USGSCSM Missing from LTS Installation #5382

jlaura opened this issue Dec 20, 2023 · 14 comments
Assignees
Labels
bug Something isn't working

Comments

@jlaura
Copy link
Collaborator

jlaura commented Dec 20, 2023

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

@jlaura jlaura added the bug Something isn't working label Dec 20, 2023
@acpaquette
Copy link
Collaborator

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
Copy link
Collaborator Author

jlaura commented Dec 20, 2023

I hesitate to include USGSCSM as a dependency for ISIS

Can you please expand on why?

@acpaquette
Copy link
Collaborator

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
Copy link
Collaborator Author

jlaura commented Dec 20, 2023

More questions and thoughts about this approach:

  • 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?
  • 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?
  • "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 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.
  • 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.

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
Copy link
Collaborator

Kelvinrr commented Dec 21, 2023

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
Copy link
Collaborator

Kelvinrr commented Dec 21, 2023

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
Copy link
Collaborator Author

jlaura commented Dec 21, 2023

@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
Copy link
Contributor

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.

Copy link

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

@github-actions github-actions bot added the inactive Issue that has been inactive for at least 6 months label Jun 24, 2024
@jlaura
Copy link
Collaborator Author

jlaura commented Jun 24, 2024

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

@github-actions github-actions bot removed the inactive Issue that has been inactive for at least 6 months label Jun 25, 2024
@acpaquette
Copy link
Collaborator

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

@acpaquette
Copy link
Collaborator

acpaquette commented Jul 31, 2024

Good progress here, USGSCSM 2.0.2 is out and works on linux as expected! This can be moved to done once an 8.0.3 release is made with USGSCSM as a dependency

@jlaura
Copy link
Collaborator Author

jlaura commented Aug 6, 2024

@acpaquette 🎉

Is this fix getting back ported into the LTS as well?

@acpaquette
Copy link
Collaborator

@jlaura I would like it to be yes. It may come out with an 8.0.4 release with a small change to the paths in the ISIS preferences file. Alternatively, we could do a new 8.0.3 build with USGSCSM as a dep and just expect anyone using it to update there preference file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: In Progress
Development

No branches or pull requests

4 participants