Conversation
nselvidge
force-pushed
the
sandbox-registration
branch
3 times, most recently
from
a81b83a to
b5268f1
Compare
nselvidge
force-pushed
the
sandbox-registration
branch
from
3fe74ea to
876ce67
Compare
did you want to make that api/interface solid first and then send up the SDK changes? |
|
Removing myself (avoids me getting pinged), but add me back as a reviewer when this is ready |
nselvidge
force-pushed
the
sandbox-registration
branch
2 times, most recently
from
1930df0 to
32af70e
Compare
js/src/logger.ts
Outdated
| * Configuration for a sandbox runtime. | ||
| * @internal | ||
| */ | ||
| export interface ExperimentalSandboxConfig { |
There was a problem hiding this comment.
can we put this stuff in a new file just to avoid further bloating this one (same w/ python)?
js/src/logger.ts
Outdated
| const state = _internalGetGlobalState(); | ||
| await login({ | ||
| apiKey: options.apiKey, | ||
| appUrl: options.appUrl, | ||
| orgName: options.orgName, |
There was a problem hiding this comment.
nowadays it's convention to accept the state as an optional param and then default it to the global state if nothing is passed in
js/src/cli/functions/infer-source.ts
Outdated
| } else if (location.type === "function") { | ||
| return `task ${location.index}`; | ||
| } | ||
| return `sandbox eval ${location.eval_name}`; |
There was a problem hiding this comment.
what is location.type expected to be for sandboxes?
There was a problem hiding this comment.
if it's sandbox then maybe we check that explicitly and return unknown otherwise?
py/src/braintrust/logger.py
Outdated
| ) | ||
| print([f.id for f in result.functions]) | ||
| """ | ||
| state = _state |
|
I'm going to hold off on merging the sdk API changes for now, I've put the typespec changes in a separate PR: #1392 |
nselvidge
force-pushed
the
sandbox-registration
branch
from
5c4d3ee to
4d9cb8c
Compare
ibolmo
left a comment
ibolmo
left a comment
There was a problem hiding this comment.
overall looks good. left just a few notes/q's
| ctx: sourceMapContext, | ||
| }), | ||
| preview: sourceMapContext | ||
| ? await findCodeDefinition({ |
There was a problem hiding this comment.
i take it this is because sandbox functions don't have source maps? could we skip the attempt to resolve the source map if we know the type of function?
| isArray, | ||
| isObject, | ||
| } from "./util"; | ||
| } from "../util/index"; |
There was a problem hiding this comment.
didn't expect these changes 🤔
| */ | ||
| export interface RegisterSandboxOptions { | ||
| /** Deprecated. Ignored. Function names are derived from discovered eval names. */ | ||
| name: string; |
There was a problem hiding this comment.
since this is net new, did we want to deprecate? you can just remove the props?
| // Get project ID via project registration | ||
| const projectResponse = await state | ||
| .appConn() | ||
| .post_json("api/project/register", { |
There was a problem hiding this comment.
#nit we should likely have a more formal interface like we've done in framework2.ts for projects and now sandboxes, but don't think you have to sort this out.
i've added this to a weekly planning discussion
3 participants
adds utility to register a sandbox with braintrust
Data plane APIs are not yet available, so these utilities won't do anything. merging them so they'll be ready when the data plane APIs are available.
this doesn't include any changes to the CLI, but that will come in a follow up PR soon.
I suspect some of this API is going to change over the next few weeks as we nail down supported integrations and workflows, so I'm marking things as experimental and preventing docs from getting generated for now.
This workflow should be stable going forward, but we'll likely add new ways to create sandboxes in addition to these.