feat: have NLP tasks read in DxReports as well as DocRefs #435
Conversation
mikix
force-pushed
the
mikix/nlp-dxreports
branch
from
7a26b11 to
fa22902
Compare
| docker compose run --rm \ | ||
| --volume $DATADIR:/in \ | ||
| cumulus-etl \ | ||
| nlp \ |
There was a problem hiding this comment.
The fact that this change was needed here but not before, tells me we are regression testing the public release instead of the current branch. We have code to build the branch's docker image... not sure why it's not being used here. I can dig into it later, but just an FYI - it's on my todo list.
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
mikix
force-pushed
the
mikix/nlp-dxreports
branch
from
fa22902 to
ddec0bc
Compare
mikix
force-pushed
the
mikix/nlp-dxreports
branch
from
ddec0bc to
a261f10
Compare
| f"Could not process answer from NLP server for DocRef {orig_docref['id']}: {exc}" | ||
| ) | ||
| self.add_error(orig_docref) | ||
| except Exception as exc: |
There was a problem hiding this comment.
I'm surprised ruff didn't flag this - do you have the generic exception rule turned off? I think it's fine, i was just expecting a manual comment to disable.
There was a problem hiding this comment.
I don't think our ruff config flags generic exceptions - I just confirmed it will flag bare excepts though (except:). I like that balance - I personally feel like generic exceptions have a undeserved reputation as a problem.
There was a problem hiding this comment.
ah ok - yeah i guess i buy that as 'I am explicitly stating my intent here'.
There was a problem hiding this comment.
Bare excepts also have the problem of catching BaseExceptions like SystemExit and KeyboardInterrupt, which usually you don't want to intercept.
But often linting tools don't like the generic version either for some programming purity reasons like "you should know what you're catching". But in the real world, you so often just want to say "hey I don't care what happened, I want to handle it and log it".
This commit adds support for dual-resource tasks and then adds DiagnosticReport to the NLP base task class. This required some vocabulary alignment, as we used "docref" a lot in places that now can take a docref or a dxreport. - "note" or "note resource": a DocRef or DxReport resource (dict) - "note text" or "text": the clinical text inside the note
mikix
force-pushed
the
mikix/nlp-dxreports
branch
from
a261f10 to
5160616
Compare
2 participants
This commit adds support for dual-resource tasks and then adds DiagnosticReport to the NLP base task class.
This required some vocabulary alignment, as we used "docref" a lot in places that now can take a docref or a dxreport.
Checklist
docs/) needs to be updated