Publisher: Exception for artist error

[iLLiCiTiT]

Changelog Description

Proposal of longly discussed issue to be able to show custom error message for artist instead of "This is not your fault", during publishing, by adding new exception PublishError.

Additional info

Added new exception PublishError. Exception has the same attributes as validation error, but can be raised at any order and always stops publishing. The error is showed in "previously" validation error report. The report also shows validation errors if is raised during validation.

Screenshot

(This is example message from traypublisher).

Testing notes:

1. Validate code changes.
2. Use PublishError with custom title to trigger error.
3. Instead of "This is not your fault" and generic message you should see the title and message you've defined in exception.
4. You can also add detail to show more information, e.g. traceback.

[BigRoy]

I've said this before - but I don't understand why this needs a new unique raised error and don't understand why this case is so special.

I feel like we should only differentiate between:

• An unknown error - which would be an actual bug.
• An issue or error that we know may happen.
  • And because we know it may happen we should put some love into 'describing' the issue, potentially offer solutions, etc.

As soon as we do know what happened we should strive to provide the best possible information to the user to clarify what the next steps are. That is what PublishValidationError currently offers, and to me - this error would basically be exactly that.

Can someone, in layman terms, explain why this is so unique that it needs a unique report page or error?

The example screenshot provided to me is exactly one that I'd love to still be able to see with the plug-in's logs (like Publish Validation Report) and with support for actions of the plug-in.

I suspect this PR only really solves the "unclear error" for Collectors, Extractors and Integrators, solely because those have been unable to generate a 'nice report' since PublishValidationError is reserved for Validators only. But, why?

Why couldn't a generic PublishError or KnownPublishError or PublishArtistError (which to me is an odd name for this one) do this?

---

Added new exception PublishArtistError, we should change the name, it is "artist facing error" more than artist error. Exception has the same attributes as validation error, but can be raised at any order and always stops publishing.

So why wouldn't we use PublishValidationError? (Which if we just dislike that name then we can just alias to PublishError for backwards compatibility, etc.)?

[iLLiCiTiT]

Why couldn't a generic PublishError or KnownPublishError or PublishArtistError (which to me is an odd name for this one) do this?

Just to be clear, I pointed that the exception name should be changed (and PublishError seems reasonable).

It is special because it does support title, description and detail. And PublishValidationError is special because it does not stop publishing.

I agree that KnownPublishError could at this point just inherit from the new exception for backwards compatibility and mark it as deprecated?
Technically PublishValidationError could also be converted that way, and behave as current PublishValidationError does??? But I would say that option to stop validation should be available.

As I'm thinking about it, right now the issue with KnownPublishError is it's name and lack of arguments, and PublishValidationError has confusing name to be used elsewhere -> poor choice of past, cannot be undone.

[BigRoy]

It is special because it does support title, description and detail. And PublishValidationError is special because it does not stop publishing.

I see - how about we add stop_publish as argument to PublishValidationError? :) And maybe have PublishError that inherits from PublishValidationError but sets that argument to True by default (or enforces it).

I'm just thinking of how we can simplify/have less instead of all the unique entries. Preferably the actual report page for these errors look 1:1 the same, unless there's a big reason that it requires a very unique view to data. But admittedly - this report page shown in the screenshots I feel still lacks the 'logs' of the plug-in that errored, which is annoying because it may hint to more better info already.

Plus, as came up before with the PublishValidationErrors, there are plenty cases where in code it's much easier to have a few logs be the information to the user instead of, in the plug-in have to maintain a list of all the errors or variables and build up a single raise error at the very end. Since it just complicates the logic to write that.

As I'm thinking about it, right now the issue with KnownPublishError is it's name and lack of arguments, and PublishValidationError has confusing name to be used elsewhere -> poor choice of past, cannot be undone.

Sure, but both could now inherit from PublishError - and then we can add deprecation warnings for those we really want to destroy completely.

[iLLiCiTiT]

I see - how about we add stop_publish as argument to PublishValidationError? :) And maybe have PublishError that inherits from PublishValidationError but sets that argument to True by default (or enforces it).

If it would be argument only for validation order, then I would 100% make it different exception, even if does not have anything new implemented. Argument would cause confusion for any other order error.

I feel still lacks the 'logs' of the plug-in that errored, which is annoying because it may hint to more better info already.

As artist, you probably should not need/see those logs.

which is annoying because it may hint to more better info already.

Isn't that about having reasonable message, instead of showing logs?

Plus, as came up before with the PublishValidationErrors, there are plenty cases where in code it's much easier to have a few logs be the information to the user instead of, in the plug-in have to maintain a list of all the errors or variables and build up a single raise error at the very end. Since it just complicates the logic to write that.

Isn't that what validation report does? Isn't the PublishError more about: Artist, this happened, and does not need any details, and PublishValidationError about: Artist, this is what happened and here are your details.

BTW I can't change KnownPublishError because it's behavior is different from what PublishError does now -> again poor choices of past.

[BigRoy]

As artist, you probably should not need/see those logs.

It's the exact same thing with the other plug-ins. Debug logs are not artist facing, the rest are. Should be the same reasoning here?

Isn't that about having reasonable message, instead of showing logs?

It may be - but building up a complex "message" can be cumbersome. It gets us into this type of "building a report" logic like here which may as well have just been self.log.error messages with an invalidation at the end. (And this really is one of the simpler ones. There have been implementations across plug-ins that did some sort of get_invalid_with_message logic, like here. Those to me should have just been logs and then raising the error or alike. Or IF they do want to stop on the first listed error, preferably raise directly from there. (which breaks the logic for the get_invalid call implementation for "Select Invalid" action though).

---

BTW I can't change KnownPublishError because it's behavior is different from what PublishError does now -> again poor choices of past.

We can mark it deprecated with a long retention though? If that's the better approach in this case?

[iLLiCiTiT]

It gets us into this type of "building a report" logic like here which may as well have just been self.log.error messages with an invalidation at the end. (And this really is one of the simpler ones. There have been implementations across plug-ins that did some sort of get_invalid_with_message logic, like here.

Both examples are validators. I agree that validation report should show logs, because they are meant for artists, and I think we already do that. BUT I believe that if PublishError is raised, it is completelly different use-case. Considering it will be used as KnownPublishError, just being able to show different text in UI, which is the intention or this PR, then I don't think artist should see any logs, and you can prove me wrong, but right now, at this moment, all use-cases that are not validation errors don't need logs for artist.

[BigRoy]

Both examples are validators. I agree that validation report should show logs, because they are meant for artists, and I think we already do that. BUT I believe that if PublishError is raised, it is completelly different use-case. Considering it will be used as KnownPublishError, just being able to show different text in UI, which is the intention or this PR, then I don't think artist should see any logs, and you can prove me wrong, but right now, at this moment, all use-cases that are not validation errors don't need logs for artist.

I get your point, I'm just not convinced we absolutely need this distinction because the message itself will still end up thrown into the artist's face. As such, each is - in a way - artist facing.

Anyway, did you make the necessary changes/refactors as you'd have liked based on our conversation? If so, then I'll give it a test run.

[dee-ynput]

TL;DR:
I agree with both analyses, partialy.
IMO, we're mixing Exception handling and process status+log.

What we try to achieve here is a nice enough message to present to Artists if the publish had to be stopped / has failed.
So it is about a failing state of the process. Which should be reflected in the log, with different information / explainations for different audiences.

An exception is meant to escalate unhandled event to higher levels so they can chose how to handle it.
This also would be reflected in the log, and in many cases in the process status. But that's not mandatory (ie retry policies).

Those two things have very different goals which should not overlap.

The plugins should log what they do and what they could not do, and let the system decide what's next by raising an Exception. The information carried in the Exception are for that system to decide what to do. Worst case scenario is showing it.
When the process has ended, its status drives what we show to the user: yeah! or Boooo!
Both these messages may have complex structures, built using log levels (which could be like Artist>TD>Warn>Error>Fatal in our case).

Now, here, we're using exception as log, which escalates part the log (but not all) and creates a second path in the logging flow mixed with handling flow.
I guess that's why the discussion is mixing "Artist Reporting" and "Process Stopping".

Let's imagine we want to also have an nice enough message to present to Artists when the publish succeeded. Or we introduce other states than SUCCEED/FAILED like "Will retry in 5mn".
Using exceptions for that does not feel right, and it would become a mess the explain.

That being said, we inherited this from previous implementations which evolved into using exception for reporting and managing only 2 possible states for the process.
Using some exception as exceptions (PublishValidationError) and some exception as "Artist facing log" (PublishError) isn't amazing but it's manageable if our intention and the docs are crytal clear about it.

So I'm not sure there's much gain in changing the whole lot for the beauty of it. At least as of today...

[MustafaJafar]

Here's a test from my side.
I wish if it were my fault 😅.
Anyways, could we convert the description into a dropdown menu?

The following image were generated by converting this runtime error to PublishError.

        raise PublishError(
            title="Render failed or interrupted",
            message=f"An Error occured while rendering {ropnode.path()}",
            description="More Info: {}".format(exc)
        )

But, you know what, I like that orange box.

[iLLiCiTiT]

Added logs view.

[BigRoy]

So, this really trips up my brain. Is there a big reason why this log versus description is inverted compared to the publish validation report? Do we really need it to differ in design and layout?

Can't this look very similar to the validation report (logs left, description right, actions top left) just so that everything stays familiar.

Also, the buttons on the bottom with "Copy to clipboard" and "Save to disk" do raise questions. Copy what to clipboard? The text above it? The logs? The full report JSON? Same goes for the "Save to disk" - save what? I know that these are the JSON reports (they are, right?) but I can imagine newcomers might not click those to share details.

I start feeling like this may be something that @mkolar just squashes in instant with a very different opinion or direction - so just tagging him to see what he says.

---

Reference images

Just for context some reference images of how the publisher UI behaves without this PR

Publish Validation Report:

Publish Success Report:

KnownPublishError before this PR

Expanding the bottom bar shows the raised message:

[mkolar]

Oh boy. Thank you @BigRoy for a ping.

I agree that if we're doing this, let's at least be consistent. It should really look the same as validation error, just with a different colour. No need to make a new style of message.

[iLLiCiTiT]

No need to make a new style of message.

It's not new style, I've reused what we already had there (nothing new added, no new style defined). Having it same as validation error is actually much more work, and without option to have the copy/save buttons there.

[MustafaJafar]

This looks much much better!

Example: Error Message

Example: Before Publishing

---

Also, I found a bug when hitting validation button or having a successful publish.

[iLLiCiTiT]

Also, I found a bug when hitting validation button

Oh, will look into it.

[iLLiCiTiT]

Should be fixed

[MustafaJafar]

This is cool!
It works on my side.
Here you are some test runs:

idle - no actions are triggered

When triggering validation only

When an error that uses PublishError happens

When an error that doesn't use PublishError happens

When a publish is successful