Skip to content

Commit ef8a157

Browse files
ehdrehdr
authored
Align null handling in protos with Python SDK (#223)
* Send environment with FunctionBindParams * Send empty GPUConfig protos when no GPU specified * Send an empty list of PortSpecs if non specified To mimic the Python SDK behavior.
1 parent 27711f0 commit ef8a157

File tree

10 files changed

+46
-37
lines changed

10 files changed

+46
-37
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ Both client libraries are pre-1.0, and they have separate versioning.
99
- Test clean-ups: ensure we always terminate Sandboxes, close ephemeral objects, etc.
1010
- Added debug logging to `CloudBucketMount` creation in Go, bringing it in line with the JS SDK.
1111
- Updated the API for creating `CloudBucketMount`s in JS, using the same `modal.cloudBucketMounts.create()` pattern as other Modal objects, bringing it in line with the Go SDK.
12+
- Aligned the way the JS/Go SDKs handle empty/missing fields in gRPC messages, so the behavior is identical to the Python SDK.
1213

1314
## modal-js/v0.5.4, modal-go/v0.5.4
1415

@@ -29,7 +30,7 @@ Both client libraries are pre-1.0, and they have separate versioning.
2930
- All Go SDK functions that take a Context will respect the timeout of the context.
3031
- Improved the error message when calling a webhook Function as a normal Function.
3132
- Allow customizing the config file path via `MODAL_CONFIG_PATH` environment variable (defaults to `~/.modal.toml`).
32-
- Add support for passing `MODAL_LOGLEVEL=debug` environment variable to also log debug logs, incl. all GRPC calls, etc.
33+
- Add support for passing `MODAL_LOGLEVEL=debug` environment variable to also log debug logs, incl. all gRPC calls, etc.
3334

3435
## modal-js/v0.5.0, modal-go/v0.5.0
3536

‎modal-go/app.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,10 @@ type App struct {
2626

2727
// parseGPUConfig parses a GPU configuration string into a GPUConfig object.
2828
// The GPU string format is "type" or "type:count" (e.g. "T4", "A100:2").
29-
// Returns nil if gpu is empty, or an error if the format is invalid.
29+
// Returns an empty config if gpu is empty, or an error if the format is invalid.
3030
func parseGPUConfig(gpu string) (*pb.GPUConfig, error) {
3131
if gpu == "" {
32-
return nil, nil
32+
return pb.GPUConfig_builder{}.Build(), nil
3333
}
3434

3535
gpuType := gpu

‎modal-go/app_test.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,9 @@ func TestParseGPUConfig(t *testing.T) {
1212

1313
config, err := parseGPUConfig("")
1414
g.Expect(err).ShouldNot(gomega.HaveOccurred())
15-
g.Expect(config).Should(gomega.BeNil())
15+
g.Expect(config).ShouldNot(gomega.BeNil())
16+
g.Expect(config.GetCount()).To(gomega.Equal(uint32(0)))
17+
g.Expect(config.GetGpuType()).To(gomega.Equal(""))
1618

1719
config, err = parseGPUConfig("T4")
1820
g.Expect(err).ShouldNot(gomega.HaveOccurred())

‎modal-go/cls.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,7 @@ func (c *Cls) bindParameters(ctx context.Context, parameters map[string]any, opt
291291
FunctionId: c.serviceFunctionID,
292292
SerializedParams: serializedParams,
293293
FunctionOptions: functionOptions,
294+
EnvironmentName: environmentName("", c.client.profile),
294295
}.Build())
295296
if err != nil {
296297
return "", fmt.Errorf("failed to bind parameters: %w", err)

‎modal-go/sandbox.go‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ func buildSandboxCreateRequestProto(appID, imageID string, params SandboxCreateP
9797
ptyInfo = defaultSandboxPTYInfo()
9898
}
9999

100-
var openPorts []*pb.PortSpec
100+
openPorts := make([]*pb.PortSpec, 0)
101101
for _, port := range params.EncryptedPorts {
102102
openPorts = append(openPorts, pb.PortSpec_builder{
103103
Port: uint32(port),
@@ -118,12 +118,9 @@ func buildSandboxCreateRequestProto(appID, imageID string, params SandboxCreateP
118118
}.Build())
119119
}
120120

121-
var portSpecs *pb.PortSpecs
122-
if len(openPorts) > 0 {
123-
portSpecs = pb.PortSpecs_builder{
124-
Ports: openPorts,
125-
}.Build()
126-
}
121+
portSpecs := pb.PortSpecs_builder{
122+
Ports: openPorts,
123+
}.Build()
127124

128125
secretIds := []string{}
129126
for _, secret := range params.Secrets {

‎modal-js/src/app.ts‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,11 +75,11 @@ export type EphemeralOptions = {
7575
/**
7676
* Parse a GPU configuration string into a GPUConfig object.
7777
* @param gpu - GPU string in format "type" or "type:count" (e.g. "T4", "A100:2")
78-
* @returns GPUConfig object or undefined if no GPU specified
78+
* @returns GPUConfig object (empty config if no GPU specified)
7979
*/
80-
export function parseGpuConfig(gpu: string | undefined): GPUConfig | undefined {
80+
export function parseGpuConfig(gpu: string | undefined): GPUConfig {
8181
if (!gpu) {
82-
return undefined;
82+
return GPUConfig.create({});
8383
}
8484

8585
let gpuType = gpu;
@@ -96,11 +96,10 @@ export function parseGpuConfig(gpu: string | undefined): GPUConfig | undefined {
9696
}
9797
}
9898

99-
return {
100-
type: 0, // Deprecated field, but required by proto
99+
return GPUConfig.create({
101100
count,
102101
gpuType: gpuType.toUpperCase(),
103-
};
102+
});
104103
}
105104

106105
/** Represents a deployed Modal App. */

‎modal-js/src/cls.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,7 @@ export class Cls {
238238
functionId: this.#serviceFunctionId,
239239
serializedParams,
240240
functionOptions,
241+
environmentName: this.#client.environmentName(),
241242
});
242243
return bindResp.boundFunctionId;
243244
}

‎modal-js/src/sandbox.ts‎

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ import {
1515
SchedulerPlacement,
1616
TunnelType,
1717
PortSpec,
18+
Resources,
19+
PortSpecs,
1820
} from "../proto/modal_proto/api";
1921
import {
2022
getDefaultClient,
@@ -190,27 +192,33 @@ export async function buildSandboxCreateRequestProto(
190192
const openPorts: PortSpec[] = [];
191193
if (params.encryptedPorts) {
192194
openPorts.push(
193-
...params.encryptedPorts.map((port) => ({
194-
port,
195-
unencrypted: false,
196-
})),
195+
...params.encryptedPorts.map((port) =>
196+
PortSpec.create({
197+
port,
198+
unencrypted: false,
199+
}),
200+
),
197201
);
198202
}
199203
if (params.h2Ports) {
200204
openPorts.push(
201-
...params.h2Ports.map((port) => ({
202-
port,
203-
unencrypted: false,
204-
tunnelType: TunnelType.TUNNEL_TYPE_H2,
205-
})),
205+
...params.h2Ports.map((port) =>
206+
PortSpec.create({
207+
port,
208+
unencrypted: false,
209+
tunnelType: TunnelType.TUNNEL_TYPE_H2,
210+
}),
211+
),
206212
);
207213
}
208214
if (params.unencryptedPorts) {
209215
openPorts.push(
210-
...params.unencryptedPorts.map((port) => ({
211-
port,
212-
unencrypted: true,
213-
})),
216+
...params.unencryptedPorts.map((port) =>
217+
PortSpec.create({
218+
port,
219+
unencrypted: true,
220+
}),
221+
),
214222
);
215223
}
216224

@@ -309,18 +317,18 @@ export async function buildSandboxCreateRequestProto(
309317
: undefined,
310318
workdir: params.workdir ?? undefined,
311319
networkAccess,
312-
resources: {
320+
resources: Resources.create({
313321
milliCpu,
314322
milliCpuMax,
315323
memoryMb,
316324
memoryMbMax,
317325
gpuConfig,
318-
},
326+
}),
319327
volumeMounts,
320328
cloudBucketMounts,
321329
ptyInfo,
322330
secretIds,
323-
openPorts: openPorts.length > 0 ? { ports: openPorts } : undefined,
331+
openPorts: PortSpecs.create({ ports: openPorts }),
324332
cloudProviderStr: params.cloud ?? "",
325333
schedulerPlacement,
326334
verbose: params.verbose ?? false,

‎modal-js/test/legacy/sandbox.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { parseGpuConfig } from "../../src/app";
33
import { buildSandboxCreateRequestProto } from "../../src/sandbox";
44
import { expect, test, onTestFinished } from "vitest";
55
import { buildContainerExecRequestProto } from "../../src/sandbox";
6-
import { PTYInfo_PTYType } from "../../proto/modal_proto/api";
6+
import { GPUConfig, PTYInfo_PTYType } from "../../proto/modal_proto/api";
77

88
test("CreateOneSandbox", async () => {
99
const app = await App.lookup("libmodal-test", { createIfMissing: true });
@@ -108,7 +108,7 @@ test("SandboxExecOptions", async () => {
108108
});
109109

110110
test("parseGpuConfig", () => {
111-
expect(parseGpuConfig(undefined)).toBeUndefined();
111+
expect(parseGpuConfig(undefined)).toEqual(GPUConfig.create({}));
112112
expect(parseGpuConfig("T4")).toEqual({
113113
type: 0,
114114
count: 1,

‎modal-js/test/sandbox.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { parseGpuConfig } from "../src/app";
33
import { buildSandboxCreateRequestProto } from "../src/sandbox";
44
import { expect, test, onTestFinished } from "vitest";
55
import { buildContainerExecRequestProto } from "../src/sandbox";
6-
import { PTYInfo_PTYType } from "../proto/modal_proto/api";
6+
import { GPUConfig, PTYInfo_PTYType } from "../proto/modal_proto/api";
77

88
test("CreateOneSandbox", async () => {
99
const app = await tc.apps.fromName("libmodal-test", {
@@ -102,7 +102,7 @@ test("SandboxExecOptions", async () => {
102102
});
103103

104104
test("parseGpuConfig", () => {
105-
expect(parseGpuConfig(undefined)).toBeUndefined();
105+
expect(parseGpuConfig(undefined)).toEqual(GPUConfig.create({}));
106106
expect(parseGpuConfig("T4")).toEqual({
107107
type: 0,
108108
count: 1,

0 commit comments

Comments
 (0)