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

MultipleRenderers - delegates rendering to multiple renderers #16

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kretes
Copy link

@kretes kretes commented Nov 30, 2021

This is a small facade over renderers that we use for e.g. render at the same time to stdout and to external monitoring system. It would be nice to have it in the library

@victorbenichoux
Copy link
Owner

Hi @kretes! Thanks for the contribution, this additional renderer makes a lot of sense 😄 .

I have a couple of thoughts about the implementation

  1. we need to check the interaction with the formats functionality (setting different formats for each tick). For example, if we have multiple StandardRenderers wrapped in a MultipleRenderer, should we set the formats for each tick in all of the StandardRenderer? Without some minor changes, the current change would make the tick(format=...) functionality not work with the MultipleRenderer.

  2. Could it make sense to allow the StandardRenderer to write to multiple streams? That would address some use cases (except combining StandardRenderer and LoggingRenderer), and it's much simpler 😄 .

  3. We should make it more accessible: e.g. make the ClockCollection accept a list of renderers as values for renderer= and internally construct a MultipleRenderer

  4. Let's add a unit test: e.g. with two StandardRenderers that write to stdout, using the same approach as test_file_rendering.

I am glad to have your opinion on this.

In any event, I will try and look into writing the test/integrate with the format before the week end!

@kretes
Copy link
Author

kretes commented Jan 4, 2022

Hello.

Sorry for the late reply!

  • we need to check the interaction with the formats functionality (setting different formats for each tick). For example, if we have multiple StandardRenderers wrapped in a MultipleRenderer, should we set the formats for each tick in all of the StandardRenderer? Without some minor changes, the current change would make the tick(format=...) functionality not work with the MultipleRenderer.

Right. The general problem is that format is a specific thing of a single/group of renderers and not a global ticktock property. So this should be somehow abstracted away, leaving Renderer interface unaware of it.

  • Could it make sense to allow the StandardRenderer to write to multiple streams? That would address some use cases (except combining StandardRenderer and LoggingRenderer), and it's much simpler smile .

This might work, but wouldn't solve our case, where we send the metrics to comet.ml

  • We should make it more accessible: e.g. make the ClockCollection accept a list of renderers as values for renderer= and internally construct a MultipleRenderer

That's an API usability extension. Can do.

  • Let's add a unit test: e.g. with two StandardRenderers that write to stdout, using the same approach as test_file_rendering.

Sounds sensible

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.

None yet

2 participants