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

Don't use legacy io #8

Merged
merged 11 commits into from
Feb 12, 2024
Merged

Don't use legacy io #8

merged 11 commits into from
Feb 12, 2024

Conversation

iLLiCiTiT
Copy link
Member

@iLLiCiTiT iLLiCiTiT commented Feb 8, 2024

Changelog Description

Reduced usage of legacy_io in the codebase.

Additional info

This change affected heavily few parts of the code.

  • Farm publishing did send session data to farm > by exploring the code it was neccessary to add AVALON_WORKDIR environment variable to env keys to avoid possible issues which was only missing key.
  • AVALON_WORKDIR was was at all places read from legacy_io.Session. This was changed to use environment variable. This is something to change in future as the environment variable may not be always up to date, especially in DCCs like TVPaint or Photoshop where is possible to change workdir without being acknowledged.
  • AssetsWidget does not expect dbcon and was modified to be operational. Was marked as deprecated, we need to create SimpleFoldersWidget as replacement.
  • TasksWidget was modified and moved to publisher, which was the only remaining tool using the widget.
  • Removed validation of AVALON_PROJECT on host install. The validation didn't make sense as the keys were always filled with None in past. Needed to remove the validation because of traypublisher which can change project live. Something to reconsider.

Testing notes:

  • Test farm publishing.
  • Traypublisher should be working.
  • Publisher should be able to select/change context (folder and task).

@MustafaJafar
Copy link
Contributor

MustafaJafar commented Feb 9, 2024

It publishes to farm as usual.
I can change task and folder in publisher.


I got an error on farm with the ayon pubish job.
Here's the abnormal part from the log.

2024-02-09 21:48:45:  0: STDOUT: DEBUG:pyblish.CollectAnatomyObject:Anatomy object collected for project "Robo".
2024-02-09 21:48:45:  0: STDOUT: ERROR:pyblish.plugin:Traceback (most recent call last):
2024-02-09 21:48:45:  0: STDOUT:   File "C:\Users\Mustafa Taher\AppData\Local\Ynput\AYON\dependency_packages\ayon_2308181924_windows.zip\dependencies\pyblish\plugin.py", line 527, in __explicit_process
2024-02-09 21:48:45:  0: STDOUT:     runner(*args)
2024-02-09 21:48:45:  0: STDOUT:   File "C:\Users\Mustafa Taher\AppData\Local\Ynput\AYON\addons\core_0.2.1-dev.1\ayon_core\plugins\publish\collect_rendered_files.py", line 148, in process
2024-02-09 21:48:45:  0: STDOUT: ayon_core.pipeline.publish.publish_plugins.KnownPublishError: Missing `AYON_PUBLISH_DATA`
2024-02-09 21:48:45:  0: STDOUT: Traceback (most recent call last):
2024-02-09 21:48:45:  0: STDOUT:   File "C:\Users\Mustafa Taher\AppData\Local\Ynput\AYON\dependency_packages\ayon_2308181924_windows.zip\dependencies\pyblish\plugin.py", line 527, in __explicit_process
2024-02-09 21:48:45:  0: STDOUT:     runner(*args)
2024-02-09 21:48:45:  0: STDOUT:   File "<string>", line 148, in process
2024-02-09 21:48:45:  0: STDOUT: ayon_core.pipeline.publish.publish_plugins.KnownPublishError: Missing `AYON_PUBLISH_DATA`
2024-02-09 21:48:45:  0: STDOUT: !!! ERR: 2024-02-09 21:48:44,919 >>> { CLI-publish }: [ Failed CollectRenderedFiles: Missing `AYON_PUBLISH_DATA` -- ('C:\\Users\\Mustafa Taher\\AppData\\Local\\Ynput\\AYON\\addons\\core_0.2.1-dev.1\\ayon_core\\plugins\\publish\\collect_rendered_files.py', 148, 'process', '') ] 

Copy link
Contributor

@MustafaJafar MustafaJafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works on my side (Houdini farm publishing + select/change context in publisher UI).
I didn't test tray publisher.
image

@BigRoy
Copy link
Collaborator

BigRoy commented Feb 12, 2024

  • AVALON_WORKDIR was was at all places read from legacy_io.Session. This was changed to use environment variable. This is something to change in future as the environment variable may not be always up to date, especially in DCCs like TVPaint or Photoshop where is possible to change workdir without being acknowledged.

It's good to note that there have been (and I think still are) where you want to run something in a session that is not the current context session. So there are reasons to have an operating environment or session that you can pass along that is not the global environment.

As such, for those cases - we'd need to maintain the possibility to pass in the custom environments/contexts.

Not saying that it's currently broken in the code - haven't looked to deep, sorry. But just wanted to remark that those might be special cases that need testing?

@iLLiCiTiT
Copy link
Member Author

As such, for those cases - we'd need to maintain the possibility to pass in the custom environments/contexts.

Not saying that it's currently broken in the code - haven't looked to deep, sorry. But just wanted to remark that those might be special cases that need testing?

I'm not aware of anything we have in any of our addons. If you know about anything, please write it here as soon as possible.

But I do agree we have to implement a way how to pass custom environments, or data, to the job, just because FTRACK_* and some other environments should not be defined in deadline. But that's for a different PR as at this moment that would be feature (or enhancement).

@BigRoy
Copy link
Collaborator

BigRoy commented Feb 12, 2024

I'm not aware of anything we have in any of our addons. If you know about anything, please write it here as soon as possible.

I'd assume it's things like Library Loader that load from a different context than current project, or the Save As dialog for the Workfiles tool when browsing into another work directory than the current asset.

So I'd say:

  • Test loading from library loader from different contexts
  • Test saving into another asset and task via workfiles and opening from another asset+task.

I think most of these areas are already implemented to receive a session argument of context class instance or dict or alike. We'll just need to make sure they don't suddenly start relying on os.environ where they were originally relying on a passed along dict.

I mean legacy_io.Session was also a global mutable dict so usages of that should be relatively ok to directly remap to os.environ I presume - unless os.environ was being updated 'separately' from that. In particular updating the session context would be something to be wary with. E.g. the file browser would need to detect what changes to apply upon a session change so its knows what to format template data with, etc. It might be that a lot of logic is now already redundant with the new work files tool, etc.

Anyway, hope this helps.


To be completely clear, I'm not aware of situations where os.environ and legacy_io.Session were deliberately NOT in sync. As far as I know they were always intended to be reliably the same, with legacy_io.Session maybe slightly more reliable since I've just seen more code updating that than os environ.

I was merely referring to the fact that using a custom context at runtime, separate from the global context, would be a use case.

@iLLiCiTiT
Copy link
Member Author

Test loading from library loader from different contexts
Test saving into another asset and task via workfiles and opening from another asset+task.

Both tools do not use session since their ayon variants, so that's covered for a long time.

It might be that a lot of logic is now already redundant with the new work files tool, etc.

I worked very hard to make it redundant 🙂 .

@iLLiCiTiT iLLiCiTiT merged commit 3225173 into develop Feb 12, 2024
2 checks passed
@iLLiCiTiT iLLiCiTiT deleted the enhancement/remove-legacy-io branch February 12, 2024 14:38
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.

3 participants