Skip to content

Conversation

@darshdinger
Copy link
Contributor

Short description of the changes:

Remove local publish_plot.py and replace with conditional imports from the external plot-publisher package to eliminate code duplication while maintaining backward compatibility.

Long description of the changes:

This PR refactors finddata's plotting functionality by removing the local duplicate implementation of plotting functions and instead importing them from the external plot-publisher package when available. The changes ensure that:

  • Conda packages include plot-publisher dependency and provide full plotting functionality
  • RPM packages remain lightweight without plotting dependencies but still import successfully
  • Existing autoreduction scripts continue to work without modification
  • Code duplication is eliminated (removed 340 lines of duplicate code)

Key Changes:

  1. Removed src/finddata/publish_plot.py (340 lines of duplicate code)
  2. Updated __init__.py with conditional imports from plot_publisher package
  3. Cleaned up dependencies in pyproject.toml:
    • Removed direct plotly dependency (handled by plot-publisher)
    • Removed explicit h2 pin (comes through urllib3 as indirect dependency)
    • Added plot-publisher>=1.0,<2.0 to conda runtime dependencies
  4. Enhanced CI testing to verify both conda and RPM scenarios
  5. Maintained RPM spec file with minimal dependencies (only urllib3, toml)

Check list for the pull request

  • I have read the [CONTRIBUTING]
  • I have read the [CODE_OF_CONDUCT]
  • I have added tests for my changes
  • I have updated the documentation accordingly

Check list for the reviewer

  • I have read the [CONTRIBUTING]
  • I have verified the proposed changes
  • best software practices
    • all internal functions have an underbar, as is python standard
    • clearly named variables (better to be verbose in variable names)
    • code comments explaining the intent of code blocks
  • All the tests are passing
  • The documentation is up to date
  • code comments added when explaining intent

Manual test for the reviewer

  1. Test conda scenario (with plot-publisher):
    pixi run python -c "import finddata; print([attr for attr in dir(finddata) if not attr.startswith('_')])"
    # Should show: ['plot1d', 'plot_heatmap', 'publish_plot']
  2. Test RPM scenario (without plot-publisher):
    # In environment without plot-publisher
    python -c "import findata: print('Import successful')"
    # Should import without errors, no plotting functions available
  3. Test backward compatibility:
    pixi run python -c "import finddata; finddata.publish_plot"
  4. Verify package builds:
    ``bash
    pixi run build-conda # Should include plot-publisher dependency
    pixi run build-sdist # Includes minimal dependencies

References

EWM # 11592

@darshdinger darshdinger changed the title Ewm11592 remove plot publisher Implement external plot-publisher package and Update Dependencies Sep 24, 2025
pyproject.toml Outdated
"h2>=4.3.0,<5",
]
# urllib3 handles h2 as an indirect dependency
dependencies = ["toml>=0.10,<0.11", "urllib3>=2.5,<3"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did urllib3 move forward it's pin for h2 to avoid the security issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the specific issue you are mentioning is CVE-2025-57804 which had an HTTP request smuggling vulnerability due to illegal characters in headers, then this was patched in h2 v4.3.0+. This most current version of urlib3 has moved forward with it's pinned version of h2 to avoid the security issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you modify the lower-bound on the urllib3 pin to specify that?

Related: I think you can drop toml from the run dependencies since it isn't used by finddata.

$ rg import src/finddata
src/finddata/cli.py
2:import json
3:import logging
4:import os
5:import sys
7:from urllib3 import PoolManager
9:from finddata import __version__
179:    import argparse  # for command line options
201:        import argcomplete  # for bash completion

src/finddata/__init__.py
3:    from ._version import __version__  # noqa: F401
10:    from plot_publisher import plot1d, plot_heatmap, publish_plot  # noqa: F401
13:    # but module will still import successfully

Copy link
Member

@peterfpeterson peterfpeterson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for adding the adapter in.

@peterfpeterson peterfpeterson merged commit 41761f1 into main Sep 26, 2025
4 checks passed
@peterfpeterson peterfpeterson deleted the ewm11592-remove-plot_publisher branch September 26, 2025 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants