Plugin user API: inapplicable attrs

[kecnry]

trafficstars

Description

This pull request implements the concept of "inapplicable attributes" for a plugin which then disables access to that attribute (method, traitlet, etc) within the user API based on conditions defined in the plugin. As a proof-of-concept, this is then implemented within the links control plugin to toggle the visibility of the "wcs_use_affine" traitlet based on the value of "link_type", but would be useful across many plugins used in jdaviz.

https://github.com/spacetelescope/jdaviz/assets/877591/03b12b8e-6290-4126-9e46-051a96de024c

This was originally implemented for #2341, but after generalizing the code so that all instruments accepted the same input arguments, was no longer necessary there, so I'm opening as its own PR in case we still would want something like this to use across other plugins.

Change log entry

• [x] Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts, list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

• [x] Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
• [ ] Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
• [ ] Do the proposed changes follow the STScI Style Guides?
• [ ] Are tests added/updated as required? If so, do they follow the STScI Style Guides?
• [ ] Are docs added/updated as required? If so, do they follow the STScI Style Guides?
• [ ] Did the CI pass? If not, are the failures related?
• [x] Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
• [ ] After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.51%. Comparing base (b715b32) to head (9ef3691). Report is 659 commits behind head on main.

Files	Patch %	Lines
jdaviz/core/user_api.py	90.90%	1 Missing :warning:

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2347   +/-   ##
=======================================
  Coverage   91.51%   91.51%           
=======================================
  Files         161      161           
  Lines       19542    19563   +21     
=======================================
+ Hits        17883    17903   +20     
- Misses       1659     1660    +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[rosteen]

I'm also conflicted on this, raising an error seems maybe a little bit too aggressive - I'm just thinking of something like Pey Lian's scenario where someone is just trying to run through a notebook and this error gets thrown and interrupts things needlessly. Likely wouldn't really come up, and I seem to remember that we've intentionally moved away from warnings...I'll have to ponder this a little more.

[kecnry]

What if the user does not know what attribute is tied to what and simply running through a notebook from a collaborator

Another option would be to provide more context information for each case (likely by storing an internal dictionary with the reasons each attribute are considered inapplicable. I also considered making dir react to this, but I think that might be too heavy-handed.

and then depending on the state (which can be non-deterministic due to the interactive nature of things), running that cell may or may not throw an error.

I don't thinks it's non-deterministic. If you made interactive changes in the plugin, you're not going to get the same results anyways, and traitlets that the script assumed would do something don't anymore - so I think an error is actually useful in that scenario. Without any UI-interaction, the same script should still produce the same results (unless the script was written before this and sets traitlets in the "wrong" order).

I do see the pros and cons though. Pros: this brings the API closer to the UI experience for both the user and for the devs, which hopefully helps to make that transition from UI to API more seemless. This tries to prevent changing a value in the API, expecting to see a change or something happen, but nothing does.... until it suddenly affects a later change. Cons: some users see errors and don't read them and think it means there is a bug in the code, not in their script (but in my opinion, this is something we shouldn't shy away from but rather try to educate). In order to provide friendly error messages, we might need to build this out more to have custom strings which adds to the dev-burden.

[pllim]

From user's POV, it is much less disruptive to call an inapplicable API and have it to be silent no-op than to throw exception.

By non-deterministic, I mean, if you mix up interactive and API usage, you cannot predict if that call is going to throw error or not. So one person running this will see no error, but a different person might. Hope that is not too confusing.