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

Latest released version currently failing R Check #211

Closed
schuemie opened this issue Sep 20, 2023 · 8 comments · Fixed by #221
Closed

Latest released version currently failing R Check #211

schuemie opened this issue Sep 20, 2023 · 8 comments · Fixed by #221
Assignees
Milestone

Comments

@schuemie
Copy link
Member

I see:

── Error ('test-spot-checks.R:207:3'): Run spot-checks at per-person level on Oracle ──
Error in `.createErrorReport(dbms, err$message, sqlStatement, errorReportFile)`: Error executing SQL:
java.sql.SQLSyntaxErrorException: ORA-00955: name is already used by an existing object

From what I can gather, on Oracle it does not clean up the temp tables (such as #cov_1), and therefore throws an error if they are created again. On platforms like Oracle, you must drop the temp tables, after using them because they are not deleted automatically when the connection is closed.

@anthonysena
Copy link
Collaborator

I think this is a configuration problem:

CDM5_ORACLE_OHDSI_SCHEMA: ${{ secrets.CDM5_ORACLE_CDM54_SCHEMA }}

Should be CDM5_ORACLE_OHDSI_SCHEMA: ${{ secrets.CDM5_ORACLE_OHDSI_SCHEMA }}. Let me attempt to patch this up and hopefully this will resolve the build issue.

@schuemie
Copy link
Member Author

schuemie commented Oct 6, 2023

I think I found it: On both SQL Server and Oracle, FeatureExtraction is expecting a VISIT_DETAIL_END_DATE field in the VISIT_DETAIL table, but that no longer exists. It is called 'VISIT_END_DATE' in the database. I'll try and modify the test data, since VISIT_DETAIL_END_DATE is what it should be.

@schuemie
Copy link
Member Author

schuemie commented Oct 7, 2023

I manage to solve the errors as mentioned above, but now a new error appears. This is mostly likely caused by this issue in vroom (used by readr). The same issue affects other HADES packages, and really needs to be solved by vroom. For the HADES-wide release we'll use the previous version of vroom

@schuemie
Copy link
Member Author

For some reason the unit tests are taking a crazy long time (> 4 hours), causing the weekly R check of main to fail (apparently, Github Action declares a fail after running for 6 hours, which seems fair ;-) )

@ginberg , @leeevans : any thoughts what could cause this?

@leeevans
Copy link

@schuemie I've sent you some pgsqltest database performance metrics, via email, that may be helpful in troubleshooting

@ginberg
Copy link
Collaborator

ginberg commented Dec 22, 2023

@schuemie I am not sure yet what is the problem. I saw that after I made the latest release, the weekly R check has worked 1 time (build took a bit more than 2 hours) and after that the build takes a lot longer. I can see that the problem is with running the tests, so maybe one of the databases is slow. The R-CMD-check-Hades does work, also on Mac OS, though it also takes long there (4h or a bit more). So, I think we need to try to speed up the tests anyway.

But looking into the differences between the weekly R-Check and the R-CDM-Check from Hades (that does not fail) I also found some differences. The CDM schema's are not the same for multiple database. Eg. for weekly R-check
CDM5_ORACLE_CDM_SCHEMA: ${{ secrets.CDM5_ORACLE_CDM_SCHEMA }}
and for the R-CDM-Check-Hades it is:
CDM5_ORACLE_CDM_SCHEMA: ${{ secrets.CDM5_ORACLE_CDM54_SCHEMA }}
Could that be a reason?

@schuemie
Copy link
Member Author

Good find about the oracle CDM schema! We should change that, but I suspect the main reason the weekly check is taking longer is because all HADES packages are being tested in the weekend. I spaced them at least 1 hour apart, but of course if FeatureExtraction takes 4 hours its unit tests likely overlap with the unit tests of 3 other packages, competing for resources. I think if we get the regular GA under an hour the weekly one will be fine too.

Could you try running the unit tests in a local R instance? That should give more information about which steps are taking the longest (you can even run the tests line by line)

@ginberg
Copy link
Collaborator

ginberg commented Dec 22, 2023

@schuemie ah, good to know. Yes that sounds like a plausible explanation why the weekly is slower than the other.
Yes, I will look into this and see if I can find which steps take long. Thanks for your input and your email!

@ginberg ginberg self-assigned this Dec 22, 2023
@ginberg ginberg added this to the V3.4.0 milestone Jan 17, 2024
@anthonysena anthonysena linked a pull request Feb 2, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants