search

ynput / ayon-hiero

Hiero addon for AYON
Apache License 2.0
3 stars 2 forks source link

Removed unnecessary type conversion #5

Closed kalisp closed 2 months ago

kalisp commented 3 months ago

Changelog Description

Replaced usage of separate tags and type conversion with ast to storing everything in single tag.json_metadata which provides typing out of the box.

Additional info

Warnings or regexes caused unnecessary slowdown each saving or publishing with large amount of tracks in a workfile. Fixed additional issue with key value of asset.

Unrelated issue, but causing some slowdown, is default DEBUG level in Pyblish plugins. It should be tied to debug command line argument, but it is not (imho).

Testing notes:

  1. publish or save Hiero workfile with created AYON publish instances.
  2. console output shouldn't contain anything like: 
    *** WRN: >>> { ayon_hiero.api.lib }: [  invalid decimal literal (<unknown>, line 1)  ] *** WRN: >>> { ayon_hiero.api.lib }: [  malformed node or string on line 1: <ast.Attribute object at 0x0000021367352DD0>  ] *** WRN: >>> { ayon_hiero.api.lib }: [  invalid syntax (<unknown>, line 1)  ]
jakubjezek001 commented 3 months ago

Could we completely revamp the Ayon data tag content, shifting from hierarchical build-in tag metadata values to a JSON dump? This might significantly speed up the type conversion process. It would also align our workflow with other editorial DCC implementations.

kalisp commented 3 months ago

I extracted content of Openpype tag to new key json_metadata which contains json. This approach should be backward compatible, but it is a question if it should be cleaned up more in this PR or it should wait for transfer to Publisher (where all OP mentions should be purged, imho).

robin-ynput commented 2 months ago

I extracted content of Openpype tag to new key json_metadata which contains json. This approach should be backward compatible, but it is a question if it should be cleaned up more in this PR or it should wait for transfer to Publisher (where all OP mentions should be purged, imho).

@MilaKudr @jakubjezek001 I'm gonna get started on the new publisher thing, should we go ahead and merge this one ? I don't mind managing more clean-up/tags discussions on my PRs moving forward.

kalisp commented 2 months ago

Actually I dont think this would help slow down issue too much.

There is massive slowdown in these functions, caused by too verbose logging.

image image

That could be solved by Publisher even without this update and it is questionable, if this should be merged altogether.

jakubjezek001 commented 2 months ago

Actually I dont think this would help slow down issue too much. That could be solved by Publisher even without this update and it is questionable, if this should be merged altogether.

Yeah, perhaps we do not need to continue on this PR if - as you have said it will be faster in New publisher implementation. There is only openion on the json data implementation and its wider flexibility. But it could be implemented in the New publisher PR by @robin-ynput

kalisp commented 2 months ago

Will be revisited after implementation of Publisher if necessary.