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.

Sign up for GitHub

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

Jump to bottom

SCRUM-3515 LinkML v2.0.0 refactor #1375

Closed
wants to merge 61 commits into from
Closed

SCRUM-3515 LinkML v2.0.0 refactor #1375

wants to merge 61 commits into from

Conversation

markquintontulloch
Copy link
Member

Known issues:

  1. Genomic/BiologicalEntity DAO searches expecting an NCBITaxonTerm for some reason!
  2. Persistence exception for multivalued allele slot annotations - could just move code that attaches subject Allele, but the same logic is working fine for multivalued allele name slot annotations (would like to understand why there's a difference before just changing the code).
  3. Still seem to be having issues resolving subject/object of different types
  4. UI changes untested and probably more required

oblodgett
oblodgett reviewed Jan 3, 2024
View reviewed changes
Copy link
Member

@oblodgett oblodgett left a comment

Choose a reason for hiding this comment

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

Ok took my most of the day to get through this PR. I have not looked at the migration yet. I am going to pull this branch and give it a shot. Maybe can speed up the migration... will run again current version of alpha.

src/main/java/org/alliancegenome/curation_api/interfaces/crud/ReferenceCrudInterface.java Outdated Show resolved Hide resolved
src/main/java/org/alliancegenome/curation_api/jobs/executors/AgmDiseaseAnnotationExecutor.java Outdated Show resolved Hide resolved
.../java/org/alliancegenome/curation_api/interfaces/crud/AffectedGenomicModelCrudInterface.java Show resolved Hide resolved
src/main/java/org/alliancegenome/curation_api/model/entities/ontology/OntologyTerm.java Show resolved Hide resolved
src/main/java/org/alliancegenome/curation_api/model/entities/Allele.java Outdated Show resolved Hide resolved
src/main/java/org/alliancegenome/curation_api/services/ontology/NcbiTaxonTermService.java Show resolved Hide resolved
src/main/java/org/alliancegenome/curation_api/services/ontology/NcbiTaxonTermService.java Outdated Show resolved Hide resolved
src/main/java/org/alliancegenome/curation_api/services/base/CurieObjectCrudService.java
return ret;
}

public E findByCurie(String curie) {
Copy link
Member

Choose a reason for hiding this comment

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

This might be better moved into the BaseDAO that way these other classes don't have to inject both the service and the dao.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought this kind of logic was better kept in the service classes rather than DAO. Also, I only wanted it accessible by the classes that inherit from CurieObject

src/main/java/org/alliancegenome/curation_api/services/OrganizationService.java
@@ -38,11 +37,11 @@ public ObjectResponse<Organization> get(String orgId) {
org = orgCacheMap.get(orgId);
} else {
Log.debug("Org not cached, caching org: (" + orgId + ")");
org = organizationDAO.find(orgId);
org = organizationDAO.find(Long.parseLong(orgId));
Copy link
Member

Choose a reason for hiding this comment

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

Might want to change the definition of this method to take a Long vs String?

Copy link
Member Author

Choose a reason for hiding this comment

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

orgCacheMap can also be keyed by abbreviation (String), so I kept it like this. Could refactor though

src/main/java/org/alliancegenome/curation_api/services/ExperimentalConditionService.java Outdated Show resolved Hide resolved
markquintontulloch and others added 19 commits January 8, 2024 13:25
@markquintontulloch markquintontulloch marked this pull request as ready for review February 1, 2024 17:17
@markquintontulloch
Copy link
Member Author

@oblodgett ES indexes are smaller than those on alpha, so that doesn't explain slow performance on my machine. Might be good if you could check this locally to see how performance is for you. I can create a dump of the migrated DB again (changed since last one).

@oblodgett
Copy link
Member

ya if you can create the dump file... i'll give it a shot.

@markquintontulloch markquintontulloch deleted the SCRUM-3515 branch February 16, 2024 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oblodgett oblodgett oblodgett left review comments

@adamgibs adamgibs Awaiting requested review from adamgibs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@markquintontulloch @oblodgett