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

Fix: Locally copied version of last published workfile is not incremented #4102

Closed
wants to merge 25 commits into from

Conversation

Tilix4
Copy link
Collaborator

@Tilix4 Tilix4 commented Nov 16, 2022

Fix 1

When copied, the local workfile version keeps the published version number, when it must be +1 to follow OP's naming convention.

Fix 2

Local workfile version's name is built from anatomy. This avoids to get workfiles with their publish template naming.

Fix 3

In the case a subset has at least two tasks with published workfiles, for example Modeling and Rigging, launching Rigging was getting the first one with the next and trying to find representations, therefore workfileModeling and trying to match the current task_name (Rigging) with the representation["context"]["task"]["name"] of a Modeling representation, which was ending up to a workfile_representation to None, and exiting the process.

Trying to find the task_name in the subset['name'] fixes it.

Fix 4

Fetch input dependencies of workfile.

@iLLiCiTiT
Copy link
Member

Shouldn't the copied file actually have version 1?

@Tilix4
Copy link
Collaborator Author

Tilix4 commented Nov 16, 2022

Shouldn't the copied file actually have version 1?

Actually it's very confusing for users when the local version don't match the published ones.

@kalisp
Copy link
Member

kalisp commented Nov 16, 2022

@iLLiCiTiT I don't think so, why? But I agree with @Tilix4 , I tested that it takes latest published workfile, but forgot that Pyblish versions up automatically after publish.

@iLLiCiTiT
Copy link
Member

Am I wrong that the prelaunch hook is actually creating first workfile if there is not any? So it is actually creating first workfile version... ?

@Tilix4
Copy link
Collaborator Author

Tilix4 commented Nov 16, 2022

Am I wrong that the prelaunch hook is actually creating first workfile if there is not any? So it is actually creating first workfile version... ?

No you'are not, but it is very confusing if you know that 9 versions have been published and when opening the last one, you get a v001, but with the content of the v009 🤯

@iLLiCiTiT
Copy link
Member

Ok but in case a workfile template should be used instead of just copying the file as is.

@Tilix4
Copy link
Collaborator Author

Tilix4 commented Nov 16, 2022

Ok but in case a workfile template should be used instead of just copying the file as is.

Isn't it handled by the workfile template hook?

@Tilix4
Copy link
Collaborator Author

Tilix4 commented Nov 17, 2022

In fact, it should use the Save as code and correcting the version number, because when copied it has the name of the published one and not the naming template of the workfile.

@Tilix4 Tilix4 force-pushed the fx_auto_dl_workfile_version_up branch from 5457580 to 30895bd Compare November 17, 2022 15:07
@Tilix4
Copy link
Collaborator Author

Tilix4 commented Nov 17, 2022

Also added a fix: in the case a subset has at least two tasks with published workfiles, for example Modeling and Rigging, launching Rigging was getting the first one with the next and trying to find representations, therefore workfileModeling and trying to match the current task_name (Rigging) with the representation["context"]["task"]["name"] of a Modeling representation, which was ending up to a workfile_representation to None, and exiting the process.

Trying to find the task_name in the subset['name'] fixes it.

@@ -1032,6 +1079,31 @@ def get_representation_by_name(
return conn.find_one(query_filter, _prepare_fields(fields))


def get_representation_by_task(
Copy link
Member

Choose a reason for hiding this comment

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

I have similar issues with this function as with match_subset_id. It make sense only in very specific use-cases and since it is in fact just a filtering function I don't see a benefit to have it in public api for entities.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Sharkitty Are you using this one in #4176 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's called in the DownloadLastWorkfile Operator.

Copy link
Member

Choose a reason for hiding this comment

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

The DownloadLastWorkfile is fully dependent on sync server so if the functions are moved to sync server it should not block it.

Copy link
Collaborator Author

@Tilix4 Tilix4 Dec 9, 2022

Choose a reason for hiding this comment

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

Is it what we are supposed to do? Moving get_representation_by_task and get_matching_subset_id to sync_server.py

@antirotor antirotor added the type: enhancement Enhancements to existing functionality label Dec 7, 2022
@Tilix4 Tilix4 force-pushed the fx_auto_dl_workfile_version_up branch from 8a4fe16 to 26cccd7 Compare December 9, 2022 08:01
@LiborBatek
Copy link
Member

@Tilix4 opps. thanks, my bad! you are right!

@ynput ynput deleted a comment from Tilix4 Dec 12, 2022
)

# Get local workfile version number
last_local_workfile_version = None
Copy link
Member

Choose a reason for hiding this comment

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

There is function get_last_workfile_with_version in openpype.pipeline.workfile which does exactly this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed with get_last_workfile_with_version

@iLLiCiTiT iLLiCiTiT requested a review from kalisp January 19, 2023 10:12
@@ -229,6 +244,203 @@ def _get_configured_sites_from_setting(module, project_name, project_setting):
return configured_sites


def get_last_published_workfile_path(
Copy link
Member

Choose a reason for hiding this comment

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

There is too much arguments in this function. Also I'm not sure if it should be a function?
At the end its just this

path = None
if workfile_representation:
    path = get_representation_path_with_anatomy(
        workfile_representation, anatomy
    )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@iLLiCiTiT
Copy link
Member

I would move the hook to sync server module.

Tilix4 and others added 22 commits March 15, 2023 09:16
* Enhancement: copy last published workfile as reusable methods (WiP)

* Added get_host_extensions method, added subset_id and las_version_doc access, added optional arguments to get_last_published_workfile

* Plugged in the new methods + minor changes

* Added docstrings, last workfile optional argument, and removed unused code

* Using new implementation to get local workfile path. Warning: It adds an extra dot to the extension which I need to fix

* Refactoring and fixed double dots

* Added match subset_id and get representation method, plus clan up

* Removed unused vars

* Fixed some rebasing errors

* delinted unchanged code and renamed get_representation into get_representation_with_task

* This time it's really delinted, I hope...

* Update openpype/modules/sync_server/sync_server.py

reprenation isn't the right spelling (:

Co-authored-by: Félix David <felixg.david@gmail.com>

* Changes based on reviews

* Fixed non imperative docstring and missing space

* Fixed another non imperative docstring

* Update openpype/modules/sync_server/sync_server.py

Fixed typo

Co-authored-by: Félix David <felixg.david@gmail.com>

Co-authored-by: Hayley GUILLOT <hayleyguillot@outlook.com>
Co-authored-by: Félix David <felixg.david@gmail.com>
* Fix: Local workfile overwrite when local version number is higher than published workfile version number (WiP)

* Changed regex search, clean up

* Readded mistakenly removed newline
Co-authored-by: Jakub Trllo <43494761+iLLiCiTiT@users.noreply.github.com>
@kalisp
Copy link
Member

kalisp commented Mar 29, 2023

Replaced by #4722

@kalisp kalisp closed this Mar 29, 2023
@ynbot ynbot added this to the next-patch milestone Mar 29, 2023
@jakubjezek001 jakubjezek001 removed this from the next-patch milestone Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community contribution type: enhancement Enhancements to existing functionality
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

9 participants