Use `PublishError` along publishing #190

MustafaJafar · 2024-12-04T16:17:56Z

Changelog Description

resolve #108

This PR implements PublishError along the publish plugins.

RuntimeError
assert
KnownPublishError
Different type of Errors along publishing

Note

This PR shouldn't add PublishError to any functions in lib. instead, it should uses try..except.. in the caller code. More info #190 (comment)

Some Screenshots

Testing notes:

Everything should still work as before
Error messages related to the changes should be better.

client/ayon_houdini/api/lib.py

client/ayon_houdini/plugins/publish/collect_render_products.py

BigRoy · 2024-12-04T18:57:17Z

client/ayon_houdini/plugins/publish/collect_usd_layers.py

+            if not isinstance(dest_data, dict):
+                raise PublishError(
+                    message="Destination must be a dict."
+                )


This provides way too little detail to be any meaningful error to any artist or alike. What destination where, for which key - and what did it get instead? We're really (I feel) going the wrong way if this can't just be a TypeError but requires a PublishError since we suddenly are introducing a super generic error much less clear? (I mean, still better than the assert, don't get me wrong)

@Illicit thoughts?

Should every error (that can be raised along the publishing) be a PublishError ?
I've copula of scattered thoughts.

Any error that is not PublishError or PublishValidationError will cause the publisher UI to show the message (It's not your fault etc).

Some errors are originated from user interactions e.g. not selecting the correct node or using a wrong colorspace. I believe these definitely should be PublishError and PublishValidationError . These errors can be fixed by the user.

Other errors like the one you highlighted above, honestly, I don't know why/how it can happen and if the user has something to do with it. I assume it requires a developer to deal with them. but using PublishError can help developers know about the problem from a screenshot. (Note: I didn't use the title in the PublishError so the publish plugin's label will be used.)

Any error that is not PublishError or PublishValidationError will cause the publisher UI to show the message (It's not your fault etc).

Yup - this was a design decision at some point. I'm still not 100% confident that it's the right one, but it did force us into writing better PublishValidationError. If now we're just going to make EVERYTHING PublishError without providing better messages I think we're in the same problematic realm and we could basically revert to allow any error be nicely reported - we'll just still need to make sure that the majority of errors we raise are very clear what do to about them.

Some errors are originated from user interactions e.g. not selecting the correct node or using a wrong colorspace. I believe these definitely should be PublishError and PublishValidationError . These errors can be fixed by the user.

I can't think of any now - but that does sound right. However, the error must be very clear, describe what is wrong and how to go about fixing it, etc.

Other errors like the one you highlighted above, honestly, I don't know why/how it can happen and if the user has something to do with it. I assume it requires a developer to deal with them. but using PublishError can help developers know about the problem from a screenshot. (Note: I didn't use the title in the PublishError so the publish plugin's label will be used.)

Some assertions in the codebase were just there - to assert for a potential edge case where it might happen... not necessarily that they would happen. That's why some might not necessarily be bad to remain asserts (or other regular errors). They were just there because we just really wanted to make sure if the code continued that something behaved a certain way.

client/ayon_houdini/plugins/publish/save_scene.py

MustafaJafar · 2024-12-04T22:53:15Z

The function contains this line is used in imprint function which is used in publishing and loading.
Should we do something about it?

ayon-houdini/client/ayon_houdini/api/lib.py

Line 574 in cacbe68

raise TypeError("Unsupported type: %r" % type(value))

client/ayon_houdini/plugins/publish/validate_frame_token.py

client/ayon_houdini/plugins/publish/collect_output_node.py

…nder_rop` and `evalParmNoFarm`

BigRoy · 2024-12-05T13:50:09Z

client/ayon_houdini/plugins/publish/collect_arnold_rop.py

+    def evalParmNoFrame(self, rop, parm, **kwargs):
+        try:
+            return evalParmNoFrame(rop, parm, **kwargs)
+        except Exception as exc:


I vaguely recall that hou.Error does not inherit from Exception - is that still the case? If so, we may need to capture hou.Error explicitly if the library function does not?

evalParmNoFrame function has assert and RuntimeError and they'll be raised before the PublishError.
I don't know if that is sufficient.

client/ayon_houdini/api/lib.py

client/ayon_houdini/plugins/publish/collect_usd_layers.py

client/ayon_houdini/plugins/publish/collect_usd_render.py

client/ayon_houdini/plugins/publish/extract_render.py

BigRoy · 2024-12-05T13:53:43Z

client/ayon_houdini/plugins/publish/extract_rop.py

+            try:
+                self.render_rop(instance)
+            except Exception as e:
+                import hou


Why import hou here?

To be able to call hou.node

client/ayon_houdini/api/usd.py

…ss` method

moonyuet

Looks good for me.

client/ayon_houdini/api/plugin.py

client/ayon_houdini/plugins/publish/validate_animation_settings.py

BigRoy · 2024-12-09T05:29:56Z

client/ayon_houdini/api/plugin.py

+    def get_output_parameter(self, node):
+        try:
+            return get_output_parameter(node)
+        except Exception as exc:


Technically we should only be capturing the TypeError here. Right?

yes. if the node.parm(...) didn't failed. and I think it won't fail.

client/ayon_houdini/api/plugin.py

client/ayon_houdini/plugins/publish/collect_karma_rop.py

BigRoy · 2024-12-09T05:38:56Z

client/ayon_houdini/plugins/publish/collect_render_colorspace.py

+        try:
+            render_products_data = ARenderProduct(aov_name)
+        except Exception as exc:
+            raise PublishError(
+                "Failed to get render products with colorspace.",
+                detail=f"{exc}"
+            )


I really doubt that raising these errors is an improvement over just letting it error. The raised errors tells me basically nothing about what caused it exactly and hence provides me as a developer nor as an artist the means to go solve it. Hence, the original error's exception must be (hopefully) clearer as to what is going on or failing.

I feel like our raised error is obfuscating the original error, no?

Also, note that you can do raise PublishError("message") from exc or raise PublishError("message").with_traceback(exc.__traceback__) although that's maybe a bit verbose. But I guess that's not raising an error in a way that looks nice in the Publisher UI.

What do we think @iLLiCiTiT @dee-ynput - are we obfuscating the errors and just making the code more complex or does this make sense? (Or should we instead still be changing how errors appear in the UI if NOT raised as PublishError so that a regular error occurring in code still looks better). I'm a bit worried we'll now be getting tons of PublishError("error occurred") like errors added to the code just to provide the stack trace in the end to the UI instead of 'keeping it away from artists'.

Are we battling our own UI decisions here?

Take a look at the changes in this PR - and what does it actually clarify to the raised errors, etc. whilst adding a lot more code for essentially raising the same error, but then as PublishError instead of the actual error that occurred.

PublishError is meant to tell artist "some information" why it crashed, so he knows why it failed (and maybe can fix it). I don't think all those errors that are re-raised as PublishError has value for artists. Some of them should stay as error as they're raised because of "code error", or the message should be more specific so artist do understand.

Example: f"Failed evaluating parameter '{parm}' on Rop node: {rop.path()}". Does this help artist in any way?

Also keep in mind that the original error (traceback) has some value for developers too...

For information, the current status of this PR aims to do the following:

It raises publish errors instead of assert and other errors (from the caller functions in publish plugins).

It shows the original error as detail in publish error.

Enhance some error messages.

Personally I believe that PublishError should be used to guide the users about what could be wrong in their scene and how to fix it. e.g. missing frames -> tells the users to ensure that all frames exist

Maybe I abused it a little bit to capture the potential errors that could happen from the called functions so that they show up nicely.

PublishError

Shortly, it is issue that can be affected by artist or TD. It shows the message to artist that gives him a gist what happened and how he can potentially fix it or ask TD to fix it (e.g. via settings). We should write the information what he should do to fix it.
Made up examples:

"Deadline settings have missing, or invalid, credentials for 'http://some.url'."

"We don't support Maya 2022, please use 2023 or newer."

"Your model is not attached to camera but you requested review."

"Scene has set up multi-layer output, but AYON does not support."

"Valid ffmpeg executable is not available."

KnownPublishError

We know what happened it is some sort of a bug/unhandled case. Artist and TD should not care because it is error that cannot be affected by them at all.

"Project name is not set in 'projectName'. Could not initialize project's Anatomy."

"Missing AYON_PUBLISH_DATA environment variable."

Any unhandled error

We also might know they can happen, but it is better to have traceback. Same as with KnownPublishError, artist or TD should not care.

should be used to guide the users about what could be wrong in their scene and how to fix it. e.g. missing frames -> tells the users to ensure that all frames exist

Yes, but we should write what he should do, if the error does not tell it. Not just show error "missing frames" with exception message.

…lugin.eval_parm_no_frame`

MustafaJafar · 2024-12-12T22:45:38Z

closing in favor of #193

refactor RuntimeError to PublishError

1ddddab

MustafaJafar added the type: enhancement Improvement of existing functionality or minor addition label Dec 4, 2024

MustafaJafar requested review from BigRoy and moonyuet December 4, 2024 16:17

MustafaJafar self-assigned this Dec 4, 2024

MustafaJafar marked this pull request as draft December 4, 2024 16:18

MustafaJafar changed the title ~~refactor RuntimeError to PublishError~~ Use PublishError along publishing Dec 4, 2024

refactor assert to PublishError

ee0745b

BigRoy reviewed Dec 4, 2024

View reviewed changes

client/ayon_houdini/api/lib.py Outdated Show resolved Hide resolved

BigRoy reviewed Dec 4, 2024

View reviewed changes

MustafaJafar added 4 commits December 4, 2024 23:48

remove redundant message argument

47cd409

better error message in SaveCurrentScene

4502143

add error description in SaveCurrentScene

6c891cf

refactor some errors to PublishError

fea6de3

MustafaJafar commented Dec 4, 2024

View reviewed changes

client/ayon_houdini/plugins/publish/validate_frame_token.py Outdated Show resolved Hide resolved

MustafaJafar marked this pull request as ready for review December 4, 2024 22:57

moonyuet reviewed Dec 5, 2024

View reviewed changes

client/ayon_houdini/plugins/publish/collect_output_node.py Show resolved Hide resolved

Use PublishError to the caller functions in publish plugins for `re…

84be3d4

…nder_rop` and `evalParmNoFarm`

BigRoy reviewed Dec 5, 2024

View reviewed changes

Move the PublishError to the collector

ece72b3

BigRoy reviewed Dec 5, 2024

View reviewed changes

client/ayon_houdini/api/usd.py Outdated Show resolved Hide resolved

MustafaJafar added 8 commits December 6, 2024 00:07

revert copy_instance_data and move the PublishError to the `proce…

65249cc

…ss` method

replace ● with regular dash -

bde4ab3

move evalParmNoFrame to HoudiniInstancePlugin

0363a57

Use PublishValidationError instead and use better log message

bb44614

remove redundant validator

452f73e

Move publishError to the caller publish plugin

ce5a314

Move the PublishError to HoudiniInstancePlugin

6de4af0

revert changes in get_top_referenced_parm

33c2bb3

Implement PublishError when calling colorspace function.

15dbe43

MustafaJafar requested review from moonyuet and BigRoy December 6, 2024 12:31

moonyuet approved these changes Dec 6, 2024

View reviewed changes

BigRoy reviewed Dec 6, 2024

View reviewed changes

client/ayon_houdini/api/plugin.py Outdated Show resolved Hide resolved

BigRoy reviewed Dec 6, 2024

View reviewed changes

client/ayon_houdini/plugins/publish/validate_animation_settings.py Outdated Show resolved Hide resolved

BigRoy reviewed Dec 9, 2024

View reviewed changes

MustafaJafar added 4 commits December 9, 2024 11:32

refactor f{exc} to str(exc)

a03e49b

add missing argument

8bb8bf7

refactor HoudiniInstancePlugin.evalParmNoFrame to `HoudiniInstanceP…

107012c

…lugin.eval_parm_no_frame`

Fix eval_parm_no_frame call

d92689e

MustafaJafar marked this pull request as draft December 9, 2024 12:32

MustafaJafar closed this Dec 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `PublishError` along publishing #190

Use `PublishError` along publishing #190

MustafaJafar commented Dec 4, 2024 •

edited

Loading

BigRoy Dec 4, 2024

MustafaJafar Dec 4, 2024

BigRoy Dec 5, 2024

MustafaJafar commented Dec 4, 2024

BigRoy Dec 5, 2024

MustafaJafar Dec 5, 2024

BigRoy Dec 5, 2024

MustafaJafar Dec 5, 2024

moonyuet left a comment

BigRoy Dec 9, 2024

MustafaJafar Dec 9, 2024

BigRoy Dec 9, 2024

iLLiCiTiT Dec 9, 2024 •

edited

Loading

MustafaJafar Dec 9, 2024

iLLiCiTiT Dec 9, 2024 •

edited

Loading

MustafaJafar commented Dec 12, 2024

Use PublishError along publishing #190

Use PublishError along publishing #190

Conversation

MustafaJafar commented Dec 4, 2024 • edited Loading

Changelog Description

Some Screenshots

Testing notes:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MustafaJafar commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moonyuet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iLLiCiTiT Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iLLiCiTiT Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

PublishError

KnownPublishError

Any unhandled error

MustafaJafar commented Dec 12, 2024

Use `PublishError` along publishing #190

Use `PublishError` along publishing #190

MustafaJafar commented Dec 4, 2024 •

edited

Loading

iLLiCiTiT Dec 9, 2024 •

edited

Loading

iLLiCiTiT Dec 9, 2024 •

edited

Loading