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

Remove beam logging in playground for Python #32740

Merged
merged 5 commits into from
Nov 4, 2024

Conversation

hjtran
Copy link
Contributor

@hjtran hjtran commented Oct 10, 2024

I get a lot of complaints about all the logging in Beam playground for python examples. It really drowns out any output that might be coming from the examples.

I attempted to just remove the logging altogether for the python examples here, though I have no way of testing it.

My rationale is that the Beam playground environment doesn't really make sense to have logging for. If something goes wrong with an example, it makes a lot more sense to just copy and paste the example into your local Beam environment and debug it there (and optionally turn on logging in your own environment).

@hjtran
Copy link
Contributor Author

hjtran commented Oct 10, 2024

R: @robertwb

Copy link
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

@hjtran
Copy link
Contributor Author

hjtran commented Oct 18, 2024

R: @lostluck ?

@aaltay
Copy link
Member

aaltay commented Oct 21, 2024

@liferoad - Who might be a good person to review this change.

My 2 cents: To be honest, I am not sure what is a good change here. People use playground to learn, and logs help learning. But too much logging does not really help with learning.

@liferoad
Copy link
Collaborator

cc @volatilemolotov

@liferoad
Copy link
Collaborator

@hjtran could you provide some examples to show the logging issue here?

@volatilemolotov
Copy link
Contributor

@hjtran @liferoad Yeah examples would be good. I agree with @aaltay on this one as logging does show some internals and could be valuable for learning the SDK. Especially if results are usually at the end of stdout and you can easily scroll to the end

@hjtran
Copy link
Contributor Author

hjtran commented Oct 21, 2024

Hey all, thanks for the reviews!

Here's an example with AggregationSum
image

The results of this example are taken from the Apache Beam Playground cache.
2024-10-02 15:17:04,163 [INFO] Using Any for unsupported type: typing.Sequence[~T]
2024-10-02 15:17:04,388 [INFO] Missing pipeline option (runner). Executing pipeline using the default runner: DirectRunner.
2024-10-02 15:17:04,574 [WARNING] Dependencies required for Interactive Beam PCollection visualization are not available, please use: `pip install apache-beam[interactive]` to install necessary dependencies to enable all data visualization features.
2024-10-02 15:17:04,574 [WARNING] You cannot use Interactive Beam features when you are not in an interactive environment such as a Jupyter notebook or ipython terminal.
2024-10-02 15:17:04,888 [INFO] Creating state cache with size 104857600
55
  • The first INFO message it's not really even clear which transform is causing this.
  • The second message is a bit distracting imo since I don't think you can even use a different runner in Playground and the user is likely here just to learn about combiners.
  • The first warning message is again imo a bit distracting as the likely user probably isn't interested in this interactive visualization from beam
  • The second warning message is again not relevant
  • The final INFO message is hard for a user to even know what it's referring to
    The final logged "55" is what actually relates to the example, but it is dwarfed by the irrelevant messages that come before it. I've gotten feedback that it's hard to tell that there's any output related to the example.

I've been using Playground as a way to teach people the Beam model and abstractions and I think this kind of logging makes it a lot harder to do that.

@liferoad
Copy link
Collaborator

@hjtran we do not have a way to control the logging levels now with Playground. With your PR, it will remove all the logs.

@hjtran
Copy link
Contributor Author

hjtran commented Oct 21, 2024

@liferoad we could adjust the levels like the way #25692 does.

I think it'd be an improvement to adjust to ERROR, but I'd still argue that these logging messages don't help with the goal of learning with Playground so we may as well remove them.

@liferoad
Copy link
Collaborator

Can we at least adjust this PR to ERROR? I think it is needed if users play with the wrong code.

@hjtran
Copy link
Contributor Author

hjtran commented Oct 22, 2024

Can we at least adjust this PR to ERROR? I think it is needed if users play with the wrong code.

Changed!

@liferoad
Copy link
Collaborator

@hjtran
Copy link
Contributor Author

hjtran commented Oct 22, 2024

Any guess as to why the "Operation was canceled"? I'm not sure how to troubleshoot this

@liferoad
Copy link
Collaborator

Any guess as to why the "Operation was canceled"? I'm not sure how to troubleshoot this

Does this run locally? :playground:backend:runLint

@robertwb
Copy link
Contributor

FWIW, I totally agree that these logs are quite unhelpful, regardless of if you're in the playground or not.

@hjtran
Copy link
Contributor Author

hjtran commented Oct 31, 2024

How do I retrigger the precommit here? Looks like @volatilemolotov fixed the issue that caused the cancellation of the precommit runs

@volatilemolotov
Copy link
Contributor

Run Playground PreCommit

@volatilemolotov
Copy link
Contributor

@hjtran It will not be able to trigger by comment now as the PR is before my updates. Easiest way is to just add a new commit and push to re-trigger.

@hjtran
Copy link
Contributor Author

hjtran commented Nov 4, 2024

Bump @liferoad
(note, I'm away from computer for the month of November after today)

@liferoad liferoad merged commit 242b0b2 into apache:master Nov 4, 2024
5 checks passed
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.

5 participants