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

CSV Ingest - fallback to folder name if folder path is not complete #28

Conversation

jrsndl
Copy link

@jrsndl jrsndl commented Sep 29, 2024

The CSV Folder Path column should be first matched against project folder paths, and the failed items agains project folder names. Maybe this should be optional so the current behavior is preserved.
See issue 27

closes #27

@jrsndl jrsndl marked this pull request as draft September 29, 2024 14:20
@jrsndl jrsndl changed the title fallback to folder name if folder path is not complete CSV Ingest - fallback to folder name if folder path is not complete Sep 29, 2024
@jrsndl jrsndl marked this pull request as ready for review October 25, 2024 08:19
@antirotor antirotor added type: enhancement Improvement of existing functionality or minor addition community Issues and PRs coming from the community members labels Nov 12, 2024
Comment on lines +461 to +469
if missing_paths:
folder_ids_by_name: Dict[str, str] = {
folder_entity["path"]: folder_entity["id"]
for folder_entity in ayon_api.get_folders(
project_name,
folder_names=missing_paths, fields={"id", "path"}
)
}
folder_ids.update(folder_ids_by_name)
Copy link
Contributor

@BigRoy BigRoy Nov 12, 2024

Choose a reason for hiding this comment

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

I think this is interesting, but I also think we should exclude any that already have / inside the folder paths to ensure we disallow implicitly matching another folder by name if a folder path was initially passed.

Also, I think we may be better off being even more explicit and allowing a dedicated folder name column instead of folder path so that if a user uses that it's clear that the column allows matching any existing folder?

@jakubjezek001 thoughts?


Note also that this code is buggy - because below we're also using folder_ids_by_path to query the task entities - however those found by folder name are not included in that lookup.

Copy link
Member

Choose a reason for hiding this comment

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

The folder path column should be sufficient. Let's not add another column if the information can be extracted from the folder path.

@jakubjezek001
Copy link
Member

This PR conflicts with the issue here: #24. The issue suggests creating nonexistent hierarchies.

The fallback mechanism tries to identify and handle misconfigured entries in the CSV, even if they are incorrect. The CSV data is assumed to be correct, and any errors indicate a failure in the previous data assembly step. The original CSV ingest design treats it as the source of truth, expecting perfect data. Therefore, nonexistent folder paths should be prepared as we do in Editorial.

@BigRoy
Copy link
Contributor

BigRoy commented Nov 12, 2024

This PR conflicts with the issue here: #24. The issue suggests creating nonexistent hierarchies.

The fallback mechanism tries to identify and handle misconfigured entries in the CSV, even if they are incorrect. The CSV data is assumed to be correct, and any errors indicate a failure in the previous data assembly step. The original CSV ingest design treats it as the source of truth, expecting perfect data. Therefore, nonexistent folder paths should be prepared as we do in Editorial.

That's clear. Having said that, it means this PR is the wrong approach and this should be closed. Yes?

@jakubjezek001
Copy link
Member

That's clear. Having said that, it means this PR is the wrong approach and this should be closed. Yes?

That is what I would suggest, but lets wait for a @jrsndl opinion.

@jakubjezek001
Copy link
Member

Closing this PR - it can be always reopened in future in case it will be needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Issues and PRs coming from the community members type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSV ingest fallback to folder name if folder path is not complete
4 participants