-
Notifications
You must be signed in to change notification settings - Fork 1
feat(SDK): Build context helpers for the python clients and SDKs #684
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
Conversation
…en creating a blueprint
| """ | ||
| Helpers for `runloop_api_client`. | ||
| """ |
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.
stop pytest from complaining when using resources from here
|
Do we want to merge the filtering that's done here (dockerignore) with the existing |
|
After today's meeting (11/21/2025) we decided that this approach is going to be hard to support and we probably -- probably -- want to enable a slightly different path that allows users to push images they build to our systems. I'm keeping this PR open for now until we get customer feedback. |
|
it's back! After looking at how daytona and modal support harbor for tb2 it seems like this is now table stakes for sandbox providers. |
src/runloop_api_client/sdk/async_.py
Outdated
| tar.add(path, arcname=".", recursive=True) | ||
| tar_buffer.seek(0) | ||
| return tar_buffer.read() | ||
| return build_docker_context_tar(path, ignore=ignore) |
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 don't think this is the right abstraction, here, why are we talking about a docker context?
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.
Also I was hoping we could just add the filter parameter to tar.add: https://docs.python.org/3/library/tarfile.html#tarfile.TarFile.add
src/runloop_api_client/sdk/async_.py
Outdated
| name: Optional[str] = None, | ||
| metadata: Optional[Dict[str, str]] = None, | ||
| ttl: Optional[timedelta] = None, | ||
| ignore: str | Path | Sequence[str] | None = None, |
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 didn't want to introduce a new type that handles DockerIgnore?
That way the logic of the docker ignore file can live in a separate class that emits patterns compatible with the tarfile ignore property?
| return pattern | ||
|
|
||
|
|
||
| def read_ignorefile(path: Optional[Path]) -> list[str]: |
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 thought we discussed making an object for this so that it encapsulates the pattern parsing.
Ideally we have a class like DockerIgnoreMatcher("path/to/dockerignore") and it implements some interface that allows us to assign it to the upload_from_dir method:
await runloop.objects.upload_from_dir(
"path/to/dir",
name = "my-context",
ignore=DockerIgnoreMatcher("path/to/dir/.dockerignore"),
)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 made an interface for ignore types so you can have different .ignore files & patterns and added a DockerIgnoreMatcher type
adam-rl
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.
It's hard to see what's used, would you mind deleting the code that is not referenced anymore?
I think the build strategy stuff, and docker context stuff is unused.
| root = root.resolve() | ||
| buf = io.BytesIO() | ||
| with tarfile.open(mode="w:gz", fileobj=buf) as tf: | ||
| for file_path in root.rglob("*"): |
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.
Why are we filtering files on the first level here? We can just call tf.add(root, arcname=".", filter=tar_filter) once right?
…t a bit more ergonomic
sid-rl
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.
mostly comments about docstrings
sid-rl
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.
LGTM!
Highlights:
.dockerignorefiles are respected. Instead of importing the entiredockerpython SDK just for this feature there's some gnarly glob and string parsing to achieve the same effect.Notes for Reviewers:
build_context=build_ctx_obj.as_build_context(),