Skip to content

Re-use generated cohort across analyses#1318

Merged
pavgra merged 20 commits into
masterfrom
issue-1088
Sep 23, 2019
Merged

Re-use generated cohort across analyses#1318
pavgra merged 20 commits into
masterfrom
issue-1088

Conversation

@pavgra

@pavgra pavgra commented Sep 1, 2019

Copy link
Copy Markdown
Contributor

Comment thread pom.xml Outdated
Comment thread src/main/java/org/ohdsi/webapi/generationcache/CleanupScheduler.java Outdated
Comment thread src/test/java/org/ohdsi/webapi/generationcache/GenerationCacheTest.java Outdated
Comment thread src/test/java/org/ohdsi/webapi/generationcache/GenerationCacheTest.java Outdated

@anthonysena anthonysena left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@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?

Comment thread pom.xml
<groupId>org.ohdsi</groupId>
<artifactId>circe</artifactId>
<version>1.8.1</version>
<version>${circe.version}</version>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we require a placeholder for the version of circe?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Curious if this too is related to unit tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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"})

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why have we moved away from making the target table a parameter to now a hard-coded value?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not clear on where this cleanup scheduler is invoked? I see the new settings in the application.properties...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Spring invokes methods marked with @Scheduled annotation automatically

@anthonysena anthonysena left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

Comment thread pom.xml
<type>jar</type>
</dependency>
<dependency>
<groupId>com.opentable.components</groupId>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comment thread src/main/resources/resources/generationcache/cohort/resultsChecksum.sql Outdated
@anthonysena

Copy link
Copy Markdown
Collaborator

@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 wivern left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What if cohort generation statement was canceled by user? Then incomplete results might be cached, don't it?

@pavgra pavgra Sep 18, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

pavgra commented Sep 18, 2019

Copy link
Copy Markdown
Contributor Author

@wivern ,

Probably we should immediately remove all caches related when source has been deleted.

Done in 26ce49d

@anthonysena

Copy link
Copy Markdown
Collaborator

@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 cohort table. Instead it appears that it relies on the cohort_generations table to get the persons in the cohort. Do I have this correct? I have no objections to this approach but wanted to confirm the behavior was expected.

@anthonysena

Copy link
Copy Markdown
Collaborator

I was able to run through a number of tests for generating cohorts, CC and pathways without issue. However, IR gave me an issue:

2019-09-19 21:49:17.286 INFO taskExecutor-3 org.springframework.batch.core.launch.support.SimpleJobLauncher -  - Job: [SimpleJob: [name=irAnalysis]] launched with the following parameters: [{jobName=IR Analysis: 2: Optum (PDW - DEV) (OPTUM-PDW), analysis_id=2, source_id=1, sessionId=hj9tmhb5, target_table=temp_cohort_hj9tmhb5, time=1568944157063, jobAuthor=anonymous}]
2019-09-19 21:49:17.714 INFO taskExecutor-3 org.springframework.batch.core.job.SimpleStepHandler -  - Executing step: [irAnalysis.createCohortTable]
2019-09-19 21:49:18.796 INFO taskExecutor-3 org.springframework.batch.core.job.SimpleStepHandler -  - Executing step: [irAnalysis.generateCohort]
2019-09-19 21:49:19.042 ERROR taskExecutor-3 org.springframework.batch.core.step.AbstractStep -  - Encountered an error executing step irAnalysis.generateCohort in job irAnalysis
java.util.concurrent.CompletionException: java.lang.NullPointerException
	at java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:273)
	at java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:280)
	at java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1592)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.NullPointerException
	at org.ohdsi.webapi.generationcache.GenerationCacheHelper.computeCacheIfAbsent(GenerationCacheHelper.java:39)
	at org.ohdsi.webapi.cohortcharacterization.GenerateLocalCohortTasklet.lambda$null$1(GenerateLocalCohortTasklet.java:91)
	at java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1590)
	... 3 more
2019-09-19 21:49:19.718 INFO taskExecutor-3 org.springframework.batch.core.launch.support.SimpleJobLauncher -  - Job: [SimpleJob: [name=irAnalysis]] completed with the following parameters: [{jobName=IR Analysis: 2: Optum (PDW - DEV) (OPTUM-PDW), analysis_id=2, source_id=1, sessionId=hj9tmhb5, target_table=temp_cohort_hj9tmhb5, time=1568944157063, jobAuthor=anonymous}] and the following status: [FAILED]

Here is the IR analysis I was using for testing: ir-issue-1088.txt.

@pavgra

pavgra commented Sep 20, 2019

Copy link
Copy Markdown
Contributor Author

confirm the behavior was expected

Yep, that's intended behavior. Once you click Generate cohort after that it will pull the cached results from the cohort_generations

@anthonysena

Copy link
Copy Markdown
Collaborator

@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:

-- Historical generations table
IF OBJECT_ID('@results_schema.cohort_generations', 'U') IS NULL
CREATE TABLE @results_schema.cohort_generations
(
	generation_id int NOT NULL,
	SUBJECT_ID bigint NOT NULL,
	cohort_start_date date NOT NULL,
	cohort_end_date date NOT NULL
);


-- Link from all generations to the most recent generation for a cohort_definition_id
IF OBJECT_ID('@results_schema.cohort_generations_ref', 'U') IS NULL
CREATE TABLE @results_schema.cohort_generations_ref
(
	generation_id int NOT NULL,
	cohort_definition_id int NOT NULL
);

ALTER TABLE @results_schema.cohort_inclusion ADD COLUMN generation_id int NOT NULL;
ALTER TABLE @results_schema.cohort_inclusion_result ADD COLUMN generation_id int NOT NULL;
ALTER TABLE @results_schema.cohort_inclusion_stats ADD COLUMN generation_id int NOT NULL;

-- Migrate the data so we can drop the cohort_definition_id from the tables above

-- Generate the generation_id sequence based on the cohort definitions in the cohort table
INSERT @results_schema.cohort_generations_ref (generation_id, cohort_definition_id)
SELECT DISTINCT ROW_NUMBER() OVER (ORDER BY cohort_definition_id)
FROM @results_schema.cohort
;

-- TODO: Update the downstream tables to use that new generation_id in place of the cohort_definition_id 
-- Might this be easier to by creating a new table, dropping the old one and then renaming (or re-creating)
-- the target table

-- This part below may not be necessary based on how we approach the step above but leaving here so we ensure that the cohort_definition_id is dropped in these tables.
ALTER TABLE @results_schema.cohort_inclusion DROP COLUMN cohort_definition_id int NOT NULL;
ALTER TABLE @results_schema.cohort_inclusion_result DROP COLUMN cohort_definition_id int NOT NULL;
ALTER TABLE @results_schema.cohort_inclusion_stats DROP COLUMN cohort_definition_id int NOT NULL;

Just curious for your thoughts on mapping over the generation_id's to the cohort_definition_ids to the cohort_inclusion* tables? I think that inserting the data into a new, temp table, dropping the old table and then re-creating the target table might be easiest but may take a while depending on the data size.

Also we should note how these new properties are used:

cache.generation.invalidAfterDays=${cache.generation.invalidAfterDays} cache.generation.cleanupInterval=${cache.generation.cleanupInterval}

@pavgra

pavgra commented Sep 23, 2019

Copy link
Copy Markdown
Contributor Author

Hmm, @anthonysena , wouldn't something as simple as this work?

-- Historical generations table
IF OBJECT_ID('@results_schema.cohort_generations', 'U') IS NULL
CREATE TABLE @results_schema.cohort_generations
(
	generation_id int NOT NULL,
	SUBJECT_ID bigint NOT NULL,
	cohort_start_date date NOT NULL,
	cohort_end_date date NOT NULL
);

-- Link from all generations to the most recent generation for a cohort_definition_id
IF OBJECT_ID('@results_schema.cohort_generations_ref', 'U') IS NULL
CREATE TABLE @results_schema.cohort_generations_ref
(
	generation_id int NOT NULL,
	cohort_definition_id int NOT NULL
); 

INSERT INTO @results_schema.cohort_generations
SELECT cohort_definition_id as generation_id, subject_id, cohort_start_date, cohort_end_date
FROM @results_schema.cohort;

INSERT INTO @results_schema.cohort_generations_ref
SELECT cohort_definition_id as generation_id, cohort_definition_id as cohort_definition_id
FROM @results_schema.cohort;

ALTER TABLE @results_schema.cohort_censor_stats RENAME cohort_definition_id TO generation_id;
ALTER TABLE @results_schema.cohort_inclusion RENAME cohort_definition_id TO generation_id;
ALTER TABLE @results_schema.cohort_inclusion_result RENAME cohort_definition_id TO generation_id;
ALTER TABLE @results_schema.cohort_inclusion_stats RENAME cohort_definition_id TO generation_id;
ALTER TABLE @results_schema.cohort_summary_stats RENAME cohort_definition_id TO generation_id;

@anthonysena

Copy link
Copy Markdown
Collaborator

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 sp_rename: https://docs.microsoft.com/en-us/sql/relational-databases/tables/rename-columns-database-engine?view=sql-server-2017. Not sure of the other DB platforms but they may also face some challenges.

@pavgra

pavgra commented Sep 23, 2019

Copy link
Copy Markdown
Contributor Author

@anthonysena , then we can go with a table replacement approach (table renaming is supported in PDW):

ALTER TABLE @results_schema.cohort_censor_stats RENAME TO old_cohort_censor_stats;

CREATE TABLE @results_schema.cohort_censor_stats (
  generation_id int NOT NULL,
  lost_count BIGINT NOT NULL
);

INSERT INTO @results_schema.cohort_censor_stats SELECT * FROM @results_schema.old_cohort_censor_stats;

DROP TABLE @results_schema.old_cohort_censor_stats;

@anthonysena

anthonysena commented Sep 23, 2019

Copy link
Copy Markdown
Collaborator

Looks like a good approach - here is the revised script:

-- Historical generations table
IF OBJECT_ID('@results_schema.cohort_generations', 'U') IS NULL
CREATE TABLE @results_schema.cohort_generations
(
	generation_id int NOT NULL,
	SUBJECT_ID bigint NOT NULL,
	cohort_start_date date NOT NULL,
	cohort_end_date date NOT NULL
);

-- Link from all generations to the most recent generation for a cohort_definition_id
IF OBJECT_ID('@results_schema.cohort_generations_ref', 'U') IS NULL
CREATE TABLE @results_schema.cohort_generations_ref
(
	generation_id int NOT NULL,
	cohort_definition_id int NOT NULL
); 

-- Establish the cohort generation references based on the cohort_definition_id's in the system
INSERT INTO @results_schema.cohort_generations
SELECT cohort_definition_id as generation_id, subject_id, cohort_start_date, cohort_end_date
FROM @results_schema.cohort;

INSERT INTO @results_schema.cohort_generations_ref
SELECT cohort_definition_id as generation_id, cohort_definition_id as cohort_definition_id
FROM @results_schema.cohort;

-- Migrate cohort_inclusion
ALTER TABLE @results_schema.cohort_inclusion RENAME TO @results_schema.old_cohort_inclusion;

CREATE TABLE @results_schema.cohort_inclusion (
  generation_id int NOT NULL,
  rule_sequence int NOT NULL,
  name varchar(255) NULL,
  description varchar(1000) NULL
);

INSERT INTO @results_schema.cohort_inclusion (generation_id, rule_sequence, name, description)
SELECT cohort_definition_id, rule_sequence, name, description 
FROM @results_schema.old_cohort_inclusion;

DROP TABLE @results_schema.old_cohort_inclusion;

-- Migrate cohort_inclusion_result
ALTER TABLE @results_schema.cohort_inclusion_result RENAME TO @results_schema.old_cohort_inclusion_result;

CREATE TABLE @results_schema.cohort_inclusion_result (
  generation_id int NOT NULL,
  mode_id int NOT NULL,
  inclusion_rule_mask bigint NOT NULL,
  person_count bigint NOT NULL
);

INSERT INTO @results_schema.cohort_inclusion_result (generation_id, mode_id, inclusion_rule_mask, person_count)
SELECT generation_id, mode_id, inclusion_rule_mask, person_count
FROM @results_schema.old_cohort_inclusion_result
;

DROP TABLE @results_schema.old_cohort_inclusion_result;

-- Migrate cohort_inclusion_stats
ALTER TABLE @results_schema.cohort_inclusion_stats RENAME TO @results_schema.old_cohort_inclusion_stats;

CREATE TABLE @results_schema.cohort_inclusion_stats (
  generation_id int NOT NULL,
  rule_sequence int NOT NULL,
  mode_id int NOT NULL,
  person_count bigint NOT NULL,
  gain_count bigint NOT NULL,
  person_total bigint NOT NULL
);

INSERT INTO @results_schema.cohort_inclusion_stats (generation_id, rule_sequence, mode_id, person_count, gain_count, person_total)
SELECT cohort_definition_id, rule_sequence, mode_id, person_count, gain_count, person_total
FROM @results_schema.old_cohort_inclusion_stats
;

DROP TABLE @results_schema.old_cohort_inclusion_stats;

-- Migrate cohort_summary_stats
ALTER TABLE @results_schema.cohort_summary_stats RENAME TO @results_schema.old_cohort_summary_stats;

CREATE TABLE @results_schema.cohort_summary_stats(
  generation_id int NOT NULL,
  mode_id int NOT NULL,
  base_count bigint NOT NULL,
  final_count bigint NOT NULL
);

INSERT INTO @results_schema.cohort_summary_stats (generation_id, mode_id, base_count, final_count)
SELECT cohort_definition_id, mode_id, base_count, final_count
FROM @results_schema.old_cohort_summary_stats
;

DROP TABLE @results_schema.old_cohort_inclusion_stats;

-- Migrate cohort_censor_stats
ALTER TABLE @results_schema.cohort_censor_stats RENAME TO @results_schema.old_cohort_censor_stats;

CREATE TABLE @results_schema.cohort_censor_stats (
  generation_id int NOT NULL,
  lost_count BIGINT NOT NULL
);

INSERT INTO @results_schema.cohort_censor_stats (generation_id, lost_count)
SELECT cohort_definition_id, lost_count 
FROM @results_schema.old_cohort_censor_stats;

DROP TABLE @results_schema.old_cohort_censor_stats;

@anthonysena anthonysena mentioned this pull request Sep 23, 2019
8 tasks

@anthonysena anthonysena left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good from my testing and our review here. Thanks!

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 this pull request may close these issues.

Re-use generated cohort across analyses

4 participants