Re-use generated cohort across analyses#1318
Conversation
Added cache cleanup scheduled service Additional logging Extracted generation_id to constant
Added proper unique indexes Simplified CohortGeneration service
anthonysena
left a comment
There was a problem hiding this comment.
@pavgra thanks for making these changes. I still need to run through some tests on this branch but I've noted a few questions while reviewing the code. For this set of changes, can you advise on the SQL required to ALTER the results schema to support this new functionality?
| <groupId>org.ohdsi</groupId> | ||
| <artifactId>circe</artifactId> | ||
| <version>1.8.1</version> | ||
| <version>${circe.version}</version> |
There was a problem hiding this comment.
Why do we require a placeholder for the version of circe?
There was a problem hiding this comment.
It's more a matter of convention which allows us to override the version via configuration / component utilizing the WebAPI
| factory.setJpaProperties(getJPAProperties()); | ||
| factory.setPackagesToScan("org.ohdsi.webapi"); | ||
| factory.setDataSource(primaryDataSource()); | ||
| factory.setDataSource(dataSource); |
There was a problem hiding this comment.
Can you comment on why you removed the call to primaryDataSource() but leave that function in this class? I'm guessing to support the loading of the unit testing DB but wanted to confirm. Also, primaryDataSource() also checks to see if the supported JDBC drivers can be loaded and this is helpful when debugging potential configuration issues. Should we move the driver check code into this function and drop primaryDataSource()?
There was a problem hiding this comment.
This is to utilize DI and to be able to override just a Primary Data Source bean so that the overriden bean is automatically utilized by the entityManagerFactory w/o need to override the entityManagerFactory as well (because currently, we reference the concrete implementation - the primaryDataSource method and overriden data source bean won't be taken into account)
| * | ||
| */ | ||
| @Configuration | ||
| @ConditionalOnProperty(prefix = "flyway", name = "enabled", matchIfMissing = true) |
There was a problem hiding this comment.
Curious if this too is related to unit tests?
There was a problem hiding this comment.
Yep, this was added when I tried to utilize H2 + Hibernate's generate-ddl feature to be able to disable Flyway. It is not actually used in the most recent version of tests but I thought that leaving such capability (to disable flyway) wouldn't harm
| */ | ||
| @Configuration | ||
| @EnableBatchProcessing | ||
| @DependsOn({"batchDatabaseInitializer"}) |
There was a problem hiding this comment.
I'm assuming this is referencing: https://docs.spring.io/spring-boot/docs/1.5.20.RELEASE/api/org/springframework/boot/autoconfigure/batch/BatchDatabaseInitializer.html. Why is this required if the Spring Batch tables are set by Flyway: https://github.com/OHDSI/WebAPI/blob/master/src/main/resources/db/migration/sqlserver/V1.0.0.1__schema-create_spring_batch.sql?
There was a problem hiding this comment.
This might be required if you decide to go for Hibernate's generate-ddl instead of Flyway scripts. The same story as in the previous comment
| Integer sourceId = Integer.valueOf(jobParams.getString("source_id")); | ||
| Source source = sourceService.findBySourceId(sourceId); | ||
| String cohortTable = jobParams.getString(TARGET_DATABASE_SCHEMA) + "." + jobParams.getString(TARGET_TABLE); | ||
| String cohortTable = jobParams.getString(TARGET_DATABASE_SCHEMA) + ".cohort"; |
There was a problem hiding this comment.
Why have we moved away from making the target table a parameter to now a hard-coded value?
There was a problem hiding this comment.
Actually, in case of the GenerationJobExecutionListener, the variable was always equal to DEFAULT_COHORT_TABLE (which is cohort) but to discover it you had to go through almost ten function calls. So, attempted to simplify it
| import java.util.List; | ||
|
|
||
| @Component | ||
| public class CleanupScheduler { |
There was a problem hiding this comment.
I'm not clear on where this cleanup scheduler is invoked? I see the new settings in the application.properties...
There was a problem hiding this comment.
Spring invokes methods marked with @Scheduled annotation automatically
anthonysena
left a comment
There was a problem hiding this comment.
I think we'll need some way to migrate existing cohort generations to this new structure. Since things will be keyed off of the generation_id, I'd imagine the migration will need to update results schema to have the entries in the cohort_generations table for any cohorts currently generated and then have the corresponding entry in the WebAPI's generation_cache table?
| <type>jar</type> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.opentable.components</groupId> |
There was a problem hiding this comment.
Very cool trying to use OpenTable Embedded PostgreSQL Component: https://github.com/opentable/otj-pg-embedded
# Conflicts: # pom.xml # src/main/resources/application.properties
|
@pavgra thanks for the time earlier to fix the checksum issue. Doing some additional tests, I put together a very simple CC: cc-1088-testing.txt. Here is the problem I face. Both cohorts have people (>500K on my data set). I've generated both cohorts via the cohort generation process before generating the CC. When I generate the CC, I only get back information for the ACE inhibitors cohort. Any ideas? |
wivern
left a comment
There was a problem hiding this comment.
Probably we should immediately remove all caches related when source has been deleted.
| } | ||
| }); | ||
| String sql = String.format(COPY_CACHED_RESULTS, SourceUtils.getTempQualifier(source), targetTable, cd.getId(), res.getSql()); | ||
| cancelableJdbcTemplate.batchUpdate(stmtCancel, sql); |
There was a problem hiding this comment.
What if cohort generation statement was canceled by user? Then incomplete results might be cached, don't it?
There was a problem hiding this comment.
Although e.g. Postgres throws an exception when batchUpdate is canceled and the exception stops caching procedure, I've added code which explicitly checks if generation has been canceled and throws an exception which crashes the caching method (see commit 2716a77 below)
|
@pavgra testing is going well with this from my side. A question: when I am generating a CC (for example) and one of the cohorts is not yet generated, I noticed that the results are not written to the result schema's |
|
I was able to run through a number of tests for generating cohorts, CC and pathways without issue. However, IR gave me an issue: Here is the IR analysis I was using for testing: ir-issue-1088.txt. |
Yep, that's intended behavior. Once you click Generate cohort after that it will pull the cached results from the |
|
@pavgra thanks for the fixes. From my testing, the caching is working well. As discussed offline, we'll need to create a migration script for the 2.8 release as part of incorporating this work into master. To that end, I started to draft up a migration script: Just curious for your thoughts on mapping over the Also we should note how these new properties are used:
|
|
Hmm, @anthonysena , wouldn't something as simple as this work? |
|
Thanks @pavgra - that does simplify the generation_ref side for sure. My only concern is the column rename operation as this is not as straight forward for other dialects. Or at least, it is not very simple for SQL Server which requires the use of a system stored proc |
|
@anthonysena , then we can go with a table replacement approach (table renaming is supported in PDW): |
|
Looks like a good approach - here is the revised script: |
anthonysena
left a comment
There was a problem hiding this comment.
Looks good from my testing and our review here. Thanks!
fixes #1088
depends on OHDSI/circe-be#93, OHDSI/ArachneCommons#202