-
Notifications
You must be signed in to change notification settings - Fork 49
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
Drush command to update chado instance to match Tripal Term expectations #1895
Drush command to update chado instance to match Tripal Term expectations #1895
Conversation
…ripal 4 and an existing chado.
…-to-prepare-an-existing-chado-database-from-tripal-3
…-chado-database-from-tripal-3
🙃 Tripal 3 defined two terms in the
In the Tripal 3 docker, when the OBO was imported, the second one became Likewise with the "Department" term, it is also in there twice. Anyway, when I import my Chado into Tripal 4, and try to prepare Chado, I am seeing
9 is the tripal_contact cv_id So what is happening is that the Chado prepare wants to rename the "TCONTACT:0000004" term from "Organization (TCONTACT:0000004)" to "Organization", then rename the "TCONTACT:0000018" term from "Organization" to "Organization(TCONTACT:0000018)". My question is, can we somehow detect this situation with this tool (Update we can after commit 8654b1c), or else modify the OBO importer to handle this case. Or third option, fix |
…-chado-database-from-tripal-3
* okay, thinking about this, it is just that you can't make the imported chado the default chado until after it is prepared, if you want things to work correctly. |
…-chado-database-from-tripal-3
…-chado-database-from-tripal-3
Additional test procedure to reproduce the situation causing the error I saw, and see if this is properly flagged after commit 2e9af7b. We will need a new docker so that these terms get checked, because this commit modified some YAML.
So that reproduces the situation that I had. Running the validation:
This does flag the situation that will violate the unique constraint when Chado is prepared, though maybe not in a super-obvious way... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is now working for me, I would approve except one change requested 😁. Need to validate that the chado schema exists in a nice way. To reproduce, misspell your chado schema name:
- Update: This is fixed with commit 9697044
I've been testing this both in the Docker (works as intended), as well as on an imported Chado schema from the i5k database. I'm running into a few issues: [ERROR] We missed a case with the dbxref for reference annotation (SBO:0000552). These are the dbxrefs we have to work
with: Array
(
[1006] => stdClass Object
(
[dbxref_id] => 1006
[accession] => 0000552
[db_name] => SO
)
) This is then followed by Error: Object of class stdClass could not be converted to string in /var/www/t4/web/modules/contrib/tripal/tripal_chado/src/Commands/ChadoCheckTermsAgainstYaml.php on line 637 The object in question is indeed not a string, but an array: stdClass Object
(
[dbxref_id] => 113276
[accession] => 0000004
[db_name] => TContact
) I'm still a little fuzzy on all of this term checking, and all of the different
is a bit of an understatement. |
My second error above was just caused by a missing reference to the actual dbxref_id value. Under the standard T4 Docker, this section of code doesn't get called, so it probably slipped under the radar, but i5k's DB triggered it. For the case that is missed, indicated by the first error, I can provide values from the database that may help cover this, but I'm not sure what would be pertinent. Also, now that the second error above isn't causing the check function to crash entirely, I run into an error on line 651 where $term_info['db_name'] doesn't exist (another case of the code never being run on a more ideal database state). Working on a fix for this now... |
@laceysanderson In the |
I definitely vote non-destructive ( !💣 ) |
I can trigger the code at line 651 to be executed with
...
...
Commit 425ec4d fixes this. Now the output will be
|
Well Lacey is busy telling people about Tripal, so I went ahead and fixed the ugly stuff that happens when the chado schema does not exist. With commit 9697044 you will now see this instead But I didn't like still seeing the red X for cognitive complexity, so with commit ddad6f5 I went ahead and split the main function into two parts to reduce the cognitive complexity (it was a pretty long function 🙃) since it did two separate things, 1. find, then 2. report |
The cv term for SO:0000552 is I see this because I don't have a "reference annotation" defined in my Tripal 3 DB Okay, the reason that I am not seeing this error is because I have more than one dbxref, and Sean only has one. The code that does this is
I am pretty sure this should be |
I know this is closed but for convenience and organization sake, I'll put this here: I just realized the issue I'm having as described in my last comment on the tripal doc issue #82 was already brought up by @dsenalik up above: #1895 (comment). In that comment, the workaround is a simple renaming. I'd like to also point out that, at least in the database I'm using, there are two "TContact"-like entries in the db table: select * from chado.db where name ilike '%contact%';
db_id | name | description | urlprefix | url
-------+----------+-----------------------------------------------------------------------------------------------------------+--------------------------------+--------------------
116 | TContact | | |
400 | TCONTACT | Tripal Contact Ontology. A temporary ontology until a more formal appropriate ontology can be identified. | cv/lookup/TCONTACT/{accession} | cv/lookup/TCONTACT In our db, we have a number of dbxrefs that point to the Interestingly, the Chado term checker flags two rows as having the dbxref in red:
and the summary:
In the database, the two dbxrefs that were flagged in red, 113276 and 113281, both point to db_id 116, the erroneous TContact from earlier. Changing this value from 116 to 400 satisfies the Chado term check. Since the functionality was mostly there, and this was subsequently merged, I can open a new issue if we think this case needs to be handled differently, at least with different language, as it wasn't immediately clear what the solution was, or if the solution doesn't necessarily work with Doug's suggestion above, which I will test. Edit: |
One thing I would like to see added is a case-insensitive check for more that one DB or CV having the same name but different case. I think I had something like that a few years back that I cleaned up, it would just be a nice warning that would be simple to add I think. That OBO_REL is odd, it would be good to know how to reproduce it because it seems suspicious, there could be a bug in there. Edit - I still have one! I have
|
Tripal 4 Core Dev Task
Issue #1698
Tripal Version: 4
Description
This PR adds a drush command that will check a specific chado instance to ensure it matches the expectations set out by Tripal 4. This is needed because chado vocabularies and cvterms often become a bit eccentric over the years ;-p Right now, we are finding that running the prepare step in Tripal 4 on a chado exported from a Tripal 3 site fails due to changes in term expectations, definitions, etc.
More specifically, the drush command
drush trp-check-terms --chado_schema=chado
would run a number of checks on the cv, db, cvterm, and dbxref tables to ensure the values associated with key Tripal terms match those defined in the Chado Content Terms YAML. This command also offers the option in some cases to fix any deviations from what is expected.Specific cases which are checked:
reports errors for terms if:
warns if the cvterm definition doesn't match what we expected.
reports but not an error if:
Testing?
I started automated testing for this!
Manual Testing via Docker
docker exec -it tripal1698 drush trp-check-terms --chado_schema=chado