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 discarded datasets when importing history from file sources using tasks #14989

Merged
merged 7 commits into from Nov 17, 2022

Conversation

davelopez
Copy link
Contributor

@davelopez davelopez commented Nov 15, 2022

Required for #14839

When importing a history archive from a file source using the tasks system with /api/histories/from_store_async all datasets appear as discarded in the new history regardless of their real state.

This adds test coverage to expose the bug. Currently investigating the fix... 🔍

Update

This PR tries to fix some issues found in this context:

  • The tag_handler instance was missing during import so it failed when items had tags.
  • The model_store_format was not passed to the import model store and was always the default tgz.
  • The default value for discarded_data in the ImportOptions was always FORCE which apparently treated every dataset as discarded independently of their real state.
  • We don't have set_external_metadata_tool in the context of tasks so, for now, we try to set the metadata directly withing the task... See 3404db4.

How to test the changes?

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@github-actions github-actions bot added area/database Galaxy's database or data access layer area/testing area/testing/integration labels Nov 15, 2022
@davelopez davelopez added kind/bug area/histories and removed area/database Galaxy's database or data access layer labels Nov 15, 2022
@davelopez
Copy link
Contributor Author

davelopez commented Nov 15, 2022

So, in this context (importing histories from file sources using tasks), it looks like we don't have a set_external_metadata_tool defined. Is it ok to skip this regenerate_imported_metadata_if_needed step then? or how should we proceed here? I'm not familiar with the implications.

self.app.datatypes_registry.set_external_metadata_tool.regenerate_imported_metadata_if_needed(

@mvdbeek
Copy link
Member

mvdbeek commented Nov 15, 2022

Hah, you've reached the limitations of Celery. Setting metadata is another celery task, and you're not supposed to call tasks from tasks. If at all possible I would create a chord, where the first task, import_model_store returns a list of datasets to set metadata on, and the chord just chains them together. Here's a chord like that that chains fetch_data, set_metadata and finish_job: https://github.com/mvdbeek/galaxy/blob/d2850975238d1369c93b9625d9bbfd5ff709689b/lib/galaxy/tools/execute.py#L182-L196

@davelopez davelopez force-pushed the fix_import_history_using_tasks branch from fb2f00b to 3404db4 Compare November 16, 2022 11:16
@davelopez davelopez marked this pull request as ready for review November 16, 2022 11:24
@github-actions github-actions bot added this to the 23.0 milestone Nov 16, 2022
@davelopez davelopez marked this pull request as draft November 16, 2022 13:41
@davelopez davelopez marked this pull request as ready for review November 16, 2022 14:22
lib/galaxy/model/store/__init__.py Show resolved Hide resolved
lib/galaxy/model/store/__init__.py Outdated Show resolved Hide resolved
@davelopez davelopez force-pushed the fix_import_history_using_tasks branch from a9e500d to eed7911 Compare November 17, 2022 10:20
Copy link
Member

@bgruening bgruening left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the test and the fixes.

@bgruening bgruening merged commit ea43465 into galaxyproject:dev Nov 17, 2022
@davelopez davelopez deleted the fix_import_history_using_tasks branch November 18, 2022 09:34
mvdbeek added a commit that referenced this pull request Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants