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

Full Proper Logging #105

Open
acbart opened this issue Jun 30, 2023 · 1 comment
Open

Full Proper Logging #105

acbart opened this issue Jun 30, 2023 · 1 comment
Assignees
Labels
enhancement New feature or request Pedal Core Issues pertaining to Reports, Submissions, and other core infrastructure

Comments

@acbart
Copy link
Collaborator

acbart commented Jun 30, 2023

I've been ignoring logging for a while, but it's time to do it better. Currently, the best we really get with logging is the following core feedback functions:

  • system_error: This is only used by TIFA and CAIT currently. It's a feedback object with neutral valence in the SYSTEM category. It's also muted :P
  • log(*items, sep=" ", **kwargs): This is not used anywhere. It's literally calling the generic feedback class with the same kind of settings as system_error. It basically just converts all the arguments to strings and joins them with spaces as the message.
  • debug(*items, **kwargs): Same as log but with a higher priority and logs each item as a separate feedback call WITHOUT converting to a string.

The resolver currently handles the SYSTEM category by just collecting all of them that are not suppressed and putting them into the FinalFeedback.systems list. I don't think even BlockPy does anything with that data, but certainly none of the resolvers dump the info to my knowledge.

It seems clear to me that we should use the logging builtin library. There's plenty of debug and info stuff to log for each tool. Plenty of warnings and errors I'm sure too. This provides a fairly convenient mechanism to standardize on.

Here are our major use cases:

  • Tools should be able to log debug/info/warning/errors that they encounter about their operation.
  • Instructors authoring scripts should be able to log stuff about the current environment situation. This is basically the existing log and debug calls that we have now. My thought is that normally those would go through whatever the environment's Handlers are.

However, I think one thing to do is to provide a FeedbackHandler (or ReportHandler?) that would easily let someone log things directly to the feedback channels as regular feedback. However, I wouldn't expect people to really want this to be turned on, unless they were quickly trying to debug something through the feedback channel. Instead, they'd want to use whatever the environment's more sophisticated logging handlers would be.

@acbart
Copy link
Collaborator Author

acbart commented Jul 4, 2023

This dovetails with error handling. What kind of errors do we have in Pedal?

  • Student code errors are things that should be transformed into feedback. Student errors should never stop the ICS.
  • System errors are bad and should probably give a productive error message.
  • Instructor Control Script errors are also bad and should probably give a productive error message.

The latter two kinds of errors should be raised, that seems clear enough. And I guess logged? Or one of the other? I need to think a bit more about what this should look like exactly. Can we think of some cases of actual errors?

It's confusing because usually catchable exceptions are because you know the user can get things into an invalid state. But what situations in Pedal allow the instructor to get things into a bad state? Let's think about each tool:

  • TIFA: This has no configuration, so that's not very exciting. It can have errors in some places, I guess, but those are things to fix in the code rather than to just raise as errors, aren't they?
  • Sandbox: Perhaps if you set up an input wrong?
  • Assertions: Ah, yes, here's something where the user can make many mistakes regarding the values, right? They could pass in something that doesn't work in a context? But shouldn't we do type changes until something works? It's simpler for some of the cases where an invalid value is more likely, I suppose (e.g., didn't pass in a valid node name to prevent_ast).

So many of Pedal's tools are unconditional executions. You just call them and let them work. That's a tricky part of all this.

@acbart acbart added enhancement New feature or request Pedal Core Issues pertaining to Reports, Submissions, and other core infrastructure labels Jul 13, 2023
@acbart acbart self-assigned this Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Pedal Core Issues pertaining to Reports, Submissions, and other core infrastructure
Projects
None yet
Development

No branches or pull requests

1 participant