-
Notifications
You must be signed in to change notification settings - Fork 23
feat(adaptor): add Sahara adaptor for OpenFn #1478
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
base: main
Are you sure you want to change the base?
Conversation
…-update feat/sahara-assets-update
…-update chore/sahara-example-file-audio_path
…g-and-status-handling Feat/sahara logging and status handling
# Conflicts: # pnpm-lock.yaml
josephjclark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for raising this! Very cool.
Most of the core time is away on vacation until January so expect a bit less activity from us over the next two weeks :)
I've started taking a look at his and left a few comments - but I'm afraid I've hit quite a big problem which we'll all need to think about it.
Refer to my comments in util.uploadFile.
Basically it looks like you've designed this adaptor around the CLI and assumed the use of the file system. But the OpenFn app - the bit that handles all the actual automation - doesn't have a file system. So I'm afraid the adaptor as designed right now will not be compatible with our system.
We'd be happy to help you work out a solution in January (the answer is already in my comments actually)
| '@openfn/language-sahara': major | ||
| --- | ||
|
|
||
| Add Sahara adaptor with axios-based upload helper, integration scripts, updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for raising a changeset! But there's actually no need for a first release, so please delete this file
| - Comprehensive test suite | ||
| - Full JSDoc documentation | ||
|
|
||
| ### Implementation Notes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this stuff more appropriate for the readme than for the changelog?
| - Initial release of Sahara (Intron Health) adaptor | ||
| - Bearer token authentication support | ||
| - **Automatic retry logic with exponential backoff** for rate limits, server errors, and network failures | ||
| - `uploadAudioFile()` operation for uploading audio files (uses axios for reliable FormData handling) ✅ **FULLY FUNCTIONAL** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fully functional bit is a bit weird - can we remove it? It sort of implies to users that the other bits NOT fully functional!
|
|
||
| ### Added | ||
|
|
||
| - Initial release of Sahara (Intron Health) adaptor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks AI generated and it's a lot more comprehensive and detailed than it needs to be, and a lot more comprehensive and detailed than our other adaptors. For the changelog we usually just list the added/changed functions, and note any breaking changes.
I'm happy to leave details in if you want them - it's your adaptor after all! This is just a note on style and best practice.
| { | ||
| "$schema": "http://json-schema.org/draft-07/schema#", | ||
| "properties": { | ||
| "baseUrl": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would users ever change this? Does Sahara have custom deployments or endpoints or anything?
|
|
||
| - **SSL Certificate**: Sahara's server has a certificate for `*.intron.health` but the endpoint is `infer.voice.intron.io`. Set `tls.rejectUnauthorized: false` in configuration to handle this server-side SSL configuration. | ||
|
|
||
| - **Testing**: 10/13 unit tests pass. Upload tests hit real API (axios bypasses undici mocks). 100% success rate with real API integration tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a weird note and not appropriate for the changelog - please remove it 🙏
Incidentally, if the note is accurate, what's up with the 3 tests that don't pass?
| uploadAudioFile({ | ||
| audio_file_name: 'test_basic_upload', | ||
| audio_file_blob: { | ||
| // Option 1: If you have a local file, you can use fs to read it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alarm bells are ringing at the mention of local paths. There is no filesystem at all in the app. You might be able to use fs inside the adaptor in a way that works with the CLI, but I have to advise that using the file system is an anti-pattern - even in the CLI
| * Options for file upload | ||
| * @typedef {Object} UploadOptions | ||
| * @public | ||
| * @property {string} audio_file_name - Name for the uploaded audio file (required) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than duplicating all the valid properties like this (which could be inaccurate and will slide out of date over time), I would recommend just linking to the relevant Sahara docs pages.
Take a look at what we did with OpenMRS: https://docs.openfn.org/adaptors/packages/openmrs-docs#get
| /** | ||
| * Helper function to upload files to Sahara API using axios | ||
| * | ||
| * Note: Uses axios instead of undici due to FormData compatibility issues in undici v6 and v7. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're welcome to use axios, it's not a problem at all. It's good to note why you've made that choice but it doesn't need a lot of justification in the comments like this.
But I have to say, I'm very surprised undici isn't working for you? undici provides the node,js implementation of fetch(), and I'd expect it to be quite robust for uploading large blobs of form data
| // File path - use stream for efficiency | ||
| const absPath = nodepath.resolve(fileValue); | ||
| const fileName = nodepath.basename(absPath); | ||
| form.append('audio_file_blob', fs.createReadStream(absPath), fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, here we get to the issue.
This file uploader will never work with the OpenFn app / Lightning. It'll only work on local machines the CLI.
The CLI is a tool to test and debug workflows, but we expect real live workflows to be automated through the app. And because the app has no file system, this function just won't work.
Supporting a local file system might be OK, as a local, undocumented debugging option. But for integration in the app you'll need to modify this helper to have data stream passed in. Typically in a workflow you would download a file from s3 or something, and then pass the stream directly.
This can result in really nice workflows: you'd use one adaptor to download the data from some server, pass the stream straight into the Sahara adaptor, and then just handle the upload. That's the kind of thing OpenFn does really well.
The big caveat there is that, at the moment, OpenFn only allows you to pass JSON objects or strings between steps. We don't really support passing streams (it might work but I'd expect problems, particularly when running through the app).
So we might need to think about this.
| * Logging utility with toggle support | ||
| * Set ENABLE_LOGGING=false in environment or pass enableLogging: false in configuration to disable | ||
| */ | ||
| export const logger = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not keen on introducting this pattern because the CLI and app already give users a bunch of ways to control an configure logging. If we're missing features I'd rather build them into our platform rather than build out a bespoke logging tool for this adaptor..
This is not a pattern I'd want to spread to other adaptors. So I'd really like to see this removed.
Summary
This PR adds the Sahara adaptor for OpenFn workflows, enabling automated voice transcription and AI-powered clinical documentation for telehealth, call center, legal, and meeting use cases.
Fixes #
Details
Add technical details of what you've changed (and why).
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to
know!):
You can read more details in our
Responsible AI Policy
Review Checklist
Before merging, the reviewer should check the following items:
production? Is it safe to release?
dev only changes don't need a changeset.