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

[23.1] Shorten job_to_output_dataset_assocation.name for discovered outputs #17147

Open
wants to merge 1 commit into
base: release_23.1
Choose a base branch
from

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Dec 7, 2023

by using an index instead of an arbitrary name.

These outputs don't exist in the tool like output datasets, so I don't think the association name attribute matters.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your 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 this to the 23.2 milestone Dec 7, 2023
@mvdbeek mvdbeek force-pushed the use_index_instead_of_identifier_for_discovered_job_to_dataset_association branch from e61a62e to 3c1e9f4 Compare December 7, 2023 14:28
by using an index instead of an arbitrary name.

These outputs don't exist in the tool like output datasets, so I don't
think the association name attribute matters.
@mvdbeek mvdbeek force-pushed the use_index_instead_of_identifier_for_discovered_job_to_dataset_association branch from 3c1e9f4 to 1e4c53b Compare December 7, 2023 15:02
@jmchilton
Copy link
Member

My guess is we use this for matching up outputs with test assertions in interactor.py for tool tests. I would expect those to fail now. This broadly seems like necessary information we're dropping here but maybe I'm missing something. Also by necessary I mean only technically necessary - this is obviously an edge case of an edge case.

@nsoranzo
Copy link
Member

From Matrix conversation, I suggested that alternatively/additionally we could also:

diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py
index 2bf193dd33..1b953d3e17 100644
--- a/lib/galaxy/model/__init__.py
+++ b/lib/galaxy/model/__init__.py
@@ -2260,7 +2260,7 @@ class JobToOutputDatasetAssociation(Base, RepresentById):
     id = Column(Integer, primary_key=True)
     job_id = Column(Integer, ForeignKey("job.id"), index=True)
     dataset_id = Column(Integer, ForeignKey("history_dataset_association.id"), index=True)
-    name = Column(String(255))
+    name = Column(TrimmedString(255))
     dataset = relationship("HistoryDatasetAssociation", lazy="joined", back_populates="creating_job_associations")
     job = relationship("Job", back_populates="output_datasets")

since we don't have any guaranteed on the length of name any way.

There were also a couple of suggestions from @jmchilton about how to improve the tools that experienced this issue at https://matrix.to/#/!NhNWKNcgINoFZdbUbY:gitter.im/$foiMoil96TB-7UQhHiFmC7mutEMD4j00l4trZuxpesc?via=gitter.im&via=matrix.org

@mvdbeek
Copy link
Member Author

mvdbeek commented Dec 12, 2023

That just makes the problem we're having in these tests not occur in the tests, not sure that's a win. If we do decide to fix this on the Galaxy side (and not just say this is the tool responsibility, which I think is also fine) we should just fix up the test interactor that does this:

        for designation, (primary_outfile, primary_attributes) in primary_datasets.items():
            primary_output = None
            for output in outputs:
                if output["name"] == f"__new_primary_file_{name}|{designation}__":
                    primary_output = output
                    break

@mvdbeek mvdbeek removed this from the 23.2 milestone Jan 11, 2024
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.

3 participants