Closed spficklin closed 2 years ago
On second thought. I'm going to remove the other Importers and create new branches, one for each one and create issues for them. I'm thinking f anyone pulls the 9.x-4.x branch for testing or development they should only have working stuff.
@spficklin hoping this isn't a major issue, testing the prepare step on this branch shows the following errors. The steps after "Loading ontologies" don't show errors.
@spficklin further resolved the above warnings!
[x] Check the Chado database. You should see entries in the cv, db, dbxref, cvterm, cvterm_dbxref, cvterm_relationship, cvtermsynonym and cvtermprop tables. [x] Check the tripal_cv_obo table you should see several ontologies listed.
UI Check: Optionally. clean out the cvterm and dbxref tables if you want to make sure terms are re-added. Go to the Administration >> Tripal >> Data Loaders page. You should see all of our normal loaders listed. Click the OBO Vocabulary Loader. Select any vocabulary in the list, and click the Import OBO File button at the bottom. It should give you a command-line command to run the job. Run it. If you deleted records in the cvterm and dbxref tables then recheck all the tables to make sure the terms were added. Test the other functions of the online form by editing an existing OBO or deleting one (deleting an OBO is a new function added in this PR).
@laceysanderson awaiting your review!
Thanks @risharde for the quick testing. I fixed the issues you noticed with the warnings.
For my environment I created a randomly named chado schema install. There was no "chado" named schema. I use this approach to test that no code makes any schema assumptions.
✅ The prepare step worked perfectly! No errors when I ran the job and all the tables matched my expectations. ❌ Next I tried the interface for importing an OBO and noticed right away that there is no way to indicate the schema... As expected, it failed immediately because it was unable to find the cv table.
2022-07-04 17:56:22
Tripal Job Launcher
Running as user 'drupaladmin'
-------------------
2022-07-04 17:56:22: Job ID 2.
2022-07-04 17:56:22: Calling: tripal_run_importer(5)
Running 'OBO Vocabulary Loader' importer
[notice] Running 'OBO Vocabulary Loader' importer
NOTE: Loading of this file is performed using a database transaction. If it fails or is terminated prematurely then all insertions and updates are rolled back and will not be found in the database
[notice] NOTE: Loading of this file is performed using a database transaction. If it fails or is terminated prematurely then all insertions and updates are rolled back and will not be found in the database
ERROR: SQLSTATE[42P01]: Undefined table: 7 ERROR: relation "cv" does not exist
LINE 1: SELECT * FROM "cv" CV
^: SELECT * FROM "cv" CV; Array
(
)
[error] ERROR: SQLSTATE[42P01]: Undefined table: 7 ERROR: relation "cv" does not exist
LINE 1: SELECT * FROM "cv" CV
^: SELECT * FROM "cv" CV; Array
(
)
[site http://default] [TRIPAL] ERROR: SQLSTATE[42P01]: Undefined table: 7 ERROR: relation "cv" does not existLINE 1: SELECT * FROM "cv" CV ^: SELECT * FROM "cv" CV; Array()
This is an easy fix though so I can jump in and adjust it since @spficklin is on holidays :-)
@laceysanderson should I retest this one? Seems you made some changes but I didn't see any further comments here, please advise when you get a chance, thanks!
Thanks for checking in @risharde. No it's not quite ready to retest. I'm running into some issues with multiple schema that I need to resolve first -hopefully this week! I'll let you know when it's ready for retest :-)
Thanks @laceysanderson, no problem, tag me when you're ready of course!
On Mon, Jul 18, 2022 at 11:23 AM Lacey-Anne Sanderson < @.***> wrote:
Thanks for checking in @risharde https://github.com/risharde. No it's not quite ready to retest. I'm running into some issues with multiple schema that I need to resolve first -hopefully this week! I'll let you know when it's ready for retest :-)
— Reply to this email directly, view it on GitHub https://github.com/tripal/t4d8/pull/230#issuecomment-1187636917, or unsubscribe https://github.com/notifications/unsubscribe-auth/AICMFI4HF5I4BPIIXGTGAQLVUVZGNANCNFSM5Z5E5EIQ . You are receiving this because you were mentioned.Message ID: @.***>
Ok, here is a summary of where I am at: These are my specific commits
The prepare step was failing at the point of trying to save the new NCIT:Subgroup term to it's idspace. Specifically, $ncit_idspace->saveTerm($ncit__subgroup);
failed with the following error:
2022-07-18 17:17:54: Calling: tripal_chado_prepare_chado(chadobooboo)
[notice] Creating Tripal Materialized Views and Custom Tables...
[notice] Loading ontologies...
[error] SQLSTATE[23502]: Not null violation: 7 ERROR: null value in column "db_id" violates not-null constraint
DETAIL: Failing row contains (3, null, C25693, , null).: INSERT INTO "chadobooboo"."dbxref" ("db_id", "accession") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1) RETURNING dbxref_id
[error] Message: Job execution failed: Failed to complete schema integration task.
SQLSTATE[23502]: Not null violation: 7 ERROR: null value in column "db_id"
violates not-null constraint
DETAIL: Failing row contains (3, null, C25693, , null).: INSERT INTO
"chadobooboo"."dbxref" ("db_id", "accession") VALUES
(:db_insert_placeholder_0, :db_insert_placeholder_1) RETURNING dbxref_id
As you can see it is using the correct schema ('chadobooboo') for the insert but does not have the right parameter list (4 parameters including nulls where there were only 2 placeholders).
When I looked into the ChadoIdSpace->saveTerm() method, it was assuming it could get the db/cv but those were returning null. I checked my chado database and as expected those were not inserted into chado yet.
This made me go back to the prepare class and look a bit higher where the vocab should have been created: $ncit_vocab = $this->getVocabulary('ncit');
. The ChadoPreparer->getVocabulary() method uses the TripalCollectionPluginManager->load/createCollection methods. My next step would be to check these further and see if they are failing to create the cv/db in the chado database for non-default chado schema.
Okay, the issue pointed out in @laceysanderson 's most recent comment has been fixed. The problem was that the plugin manager for ID spaces/vocabularies does not call the create
function if it already knows about the IDSpace or Vocabulary. The create function adds a record to Chado. So, on the prepare of the first Chado schema it calls create but on the prepare of the second Chado schema it doesn't call create
because it already knows about it.
I fixed it by adding a recordExists
function to the interface for the collections. The plugin manager will then call that function and if the record doesn't exist (either on the create, load or delete) then it will act accordingly.
@laceysanderson it is ready for you to check that the issue goes away for you.
This is a huge addition!! I'm excited to see this merged. Thanks @laceysanderson for the review!
Issue #109
This PR contains fully functional TripalImporter class and the OBO Loader. I picked up from @risharde on Friday form his
test/loader_compiled_with_gff_upgrade
branch. He indicated this was the most up-to-date branch for the loader. I picked this up because we are in need of being able to load ontology terms to make fields.Work has gone into the
tv4g4-4-tripalImporterReviewed
and I'm not sure what is fully in that branch but I assume it overlaps with this one. As @risharde was working on thetest/loader_compiled_with_gff_upgrade
that's where I started from. I suspect this PR will now conflict with thattv4g4-4-tripalImporterReviewed
and it cannot be merged. But I think some of it was already in thetest/loader_compiled_with_gff_upgrade
so we may be okay. @risharde thoughts?What's Changed?
This PR contains the following updates to the
tv4g4-4-tripalImporterReviewed
branch.tv4g4-109-OBO_Loader
tripal/includes
directory) to a proper plugin. The importer Plugin is here:tripal/src/TripalImporter
. It has the propertAnnotion
,Interfaces
andPluginManager
directories.TripalImporter
plugin annotations and are no longer static variables. Anyone who creates a new importer will need to use the annotations to set those values.TripalImporterBase
class (also found intripal/src/TripalImporter
). All importers should extend this class.ChadoImporterBase
class (found intripal_chado/src/TripalImporter
). All Chado-specific importers should extend this class.ChadoImporterBase
class provides that for all Chado-specific importers.ChadoRecord
class is deprecated, I replaced all calls toChadoRecord
that were in the OBO loader and theTripalImporter
class with Tripal DBX calls.Administration
>>Tripal
>>Data Loaders
but they are not properly coded up. I would recommend we do the same as with the OBO loader and copy those over from Tripal v3 to pick up any recent changes rather than try to fix what's there.How to Test
Edit: Originally the other loaders (e.g. GFF, Newick) were part of this PR but I have removed them and will create separate issues and branches and add the code for those into separate branches.
There are no functional tests for this PR. But, you can test it by
tripal
andtripal_chado
modules or start from a brand new installation. This will install thetripal_cv_obo
table.cv
,db
,dbxref
,cvterm
,cvterm_dbxref
,cvterm_relationship
,cvtermsynonym
andcvtermprop
tables.tripal_cv_obo
table you should see several ontologies listed.cvterm
anddbxref
tables if you want to make sure terms are re-added.Administration
>>Tripal
>>Data Loaders
page. You should see all of our normal loaders listed.OBO Vocabulary Loader
.Import OBO File
button at the bottom. It should give you a command-line command to run the job. Run it.cvterm
anddbxref
tables then recheck all the tables to make sure the terms were added.