Skip to content

Conversation

@3nprob
Copy link

@3nprob 3nprob commented Oct 27, 2025

  • install.sh: Add --with-matplotlib flag to also install matplotlib for graph support
  • docker: install matplotlib if build-arg WITH_MATPLOTLIB=1
  • pin matplotlib like other python dependencies

3np added 3 commits October 27, 2025 09:26
needed by matplotlib;
removed alongside python3-pip if not explicitly installed
@3nprob 3nprob changed the title feat(install,docker): with-matplotlib install, docker: --with-matplotlib Oct 27, 2025
@3nprob 3nprob marked this pull request as ready for review October 27, 2025 09:53
Copy link
Contributor

@roshii roshii left a comment

Choose a reason for hiding this comment

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

nACK: afaict matplotlib is nor a project dependency, nor used in any dev flow

@3nprob
Copy link
Author

3nprob commented Oct 28, 2025

nACK: afaict matplotlib is nor a project dependency, nor used in any dev flow

It's used by ob-watcher to plot graphs. Without it you get an error telling you to please install matplotlib when navigating to some views.

https://github.com/JoinMarket-Org/joinmarket-clientserver/blame/5246c1ccb5fcc9eae383e8addcca3677bb9e76c8/scripts/obwatch/ob-watcher.py#L32-L36

https://github.com/JoinMarket-Org/joinmarket-clientserver/blame/5246c1ccb5fcc9eae383e8addcca3677bb9e76c8/scripts/obwatch/ob-watcher.py#L259C13-L260

joinmarket-webui/jam#182

So "user flow" more than "dev flow" - it's already an undocumented optional runtime dependency.

@3nprob 3nprob requested a review from roshii October 28, 2025 19:37
Copy link
Contributor

@roshii roshii left a comment

Choose a reason for hiding this comment

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

I missed that usage, but remain concern by adding matplotlib to project dependencies, which it is not one per se, and it's an heavy one.

I'd suggest wrapping ob-watcher into a shell script, which is also arguable provided the ob-watcher handles missing matplotlib gracefully, and can be resolved by a single pip install.

nACK

@3nprob
Copy link
Author

3nprob commented Oct 29, 2025

@roshii

Note that here matplotlib is still not installed by default for non-docker invocations and requires passing --with-matplotlib flag to install.sh for it to be installed.

I'd suggest wrapping ob-watcher into a shell script, which is also arguable provided the ob-watcher handles missing matplotlib gracefully, and can be resolved by a single pip install.

I think downloading and installing libraries dynamically at runtime is an antipattern and should be avoided for a myriad of reasons. "Can be resolved by a single pip install" is not as trivial for docker users.

For docker, I see a 30% image size increase with this PR vs master.

(So if coupled with #1800 we are still net-negative ;))

@roshii
Copy link
Contributor

roshii commented Oct 29, 2025

here's how i'd tackle it: roshii#6

Missing a little configuration step to get it running out of the box

@3nprob
Copy link
Author

3nprob commented Oct 30, 2025

Update: matplotlib is now opt-in for docker too.

Enable with --build-arg WITH_MATPLOTLIB=1

@3nprob 3nprob requested a review from roshii November 1, 2025 03:41
@3nprob 3nprob mentioned this pull request Nov 1, 2025
Copy link
Contributor

@roshii roshii left a comment

Choose a reason for hiding this comment

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

while this is better, i am not in favor of this approach. let's not add non direct project dependencies, even conditionally.

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.

2 participants