Temp draft PR for easier diffing #256
Conversation
| @@ -28,13 +26,6 @@ public AiExpressionCache() throws IOException { | |||
| super(DEFAULT_CACHE_DIR, DEFAULT_TIMEOUT_MILLIS, DEFAULT_POLL_FREQUENCY_MILLIS); | |||
| } | |||
|
|
|||
| // Compute SHA-256 hash digest of input | |||
| private static String computeDigest(String input) throws NoSuchAlgorithmException { | |||
There was a problem hiding this comment.
We might need to get this back?
There was a problem hiding this comment.
Sorry, we're not using this any more - I should have deleted it.
| public static List<JSONObject> processExpressionData(RecordInstance geneRecord, int maxExperiments) throws WdkModelException, WdkUserException { | ||
| // return value: | ||
| List<JSONObject> experiments = new ArrayList<>(); | ||
| public static GeneSummaryInputs getSummaryInputsFromRecord(RecordInstance record, Function<JSONObject, String> experimentDigester) throws WdkModelException { |
There was a problem hiding this comment.
Could we call this getExperimentPrompt to be consistent and not confuse with md5 digesting?
| * @param summaryInputs inputs | ||
| * @return response JSON (indicating cache hit or not with data or miss reason respectively) | ||
| * @throws Exception lookup or other exception if unable to find or validate cached data | ||
| */ |
There was a problem hiding this comment.
We only use this function as a visitor function passed to _cache.populateAndProcessContent() which is good because getValidStoredData() accesses the cache directory directly (via digestsMatch() and readCachedData(), so we need the lock in place.
Is there some way to stop us accidentally calling these methods/functions outside of the lock mechanism? Or is this not really necessary if they are private methods? Maybe we just need an extra line in the documentation to explain this?
| List<ExperimentInputs> getExperimentsWithData(); | ||
|
|
||
| default String getDigest() { | ||
| // TODO Does it make more sense to md5 the concatenation of the experiment hashes? |
There was a problem hiding this comment.
Yes, it'll save a bit of compute too!
There was a problem hiding this comment.
Ideally this needs to digest the second level (final summary) prompt as well as the inputs to that prompt. Then when we tweak the prompting on the live site, cache entries will be invalidated automatically. I am about to push the fix for this.
| List<JSONObject> experiments = new ArrayList<>(); | ||
| // start with serial generation; move back to parallel later | ||
| for (ExperimentInputs input : experimentData) { | ||
| experiments.add(_cache.populateAndProcessContent(input.getCacheKey(), |
There was a problem hiding this comment.
I think this nested populateAndProcessContent() is going to fail to get a lock?
| private static final String CACHE_DIR_PROP_NAME = "AI_EXPRESSION_CACHE_DIR"; | ||
| private static final String DEFAULT_TMP_CACHE_SUBDIR = "expressionCache"; | ||
|
|
||
| // catch characteristics | ||
| private static final long DEFAULT_TIMEOUT_MILLIS = 5000; |
There was a problem hiding this comment.
I know this is just a detail, but let's say the AI summarisation takes up to 3 minutes and is performed within OnDiskCache.populateAndProcessContent(). Does that mean the timeout should be at least 3 × 60 × 1000?
No description provided.