Skip to content

Commit ab3ca8f

Browse files
committed
Remove enableLegacyFilename
Signed-off-by: ddl-rliu <richard.liu@dominodatalab.com>
1 parent 9327eb0 commit ab3ca8f

22 files changed

Lines changed: 190 additions & 305 deletions

File tree

flytecopilot/cmd/download_test.go

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -157,10 +157,10 @@ func TestDownloadOptions_Download(t *testing.T) {
157157
iface := &core.VariableMap{
158158
Variables: map[string]*core.Variable{
159159
"blob": {
160-
Type: &core.LiteralType{Type: &core.LiteralType_Blob{Blob: &core.BlobType{Dimensionality: core.BlobType_SINGLE, Format: "xyz", FileExtension: "xyz", EnableLegacyFilename: false}}},
160+
Type: &core.LiteralType{Type: &core.LiteralType_Blob{Blob: &core.BlobType{Dimensionality: core.BlobType_SINGLE, Format: "xyz", FileExtension: "xyz"}}},
161161
},
162-
"legacy_blob": {
163-
Type: &core.LiteralType{Type: &core.LiteralType_Blob{Blob: &core.BlobType{Dimensionality: core.BlobType_SINGLE, Format: "xyz", FileExtension: "xyz", EnableLegacyFilename: true}}},
162+
"csv_blob": {
163+
Type: &core.LiteralType{Type: &core.LiteralType_Blob{Blob: &core.BlobType{Dimensionality: core.BlobType_SINGLE, Format: "csv", FileExtension: "csv"}}},
164164
},
165165
},
166166
}
@@ -182,27 +182,25 @@ func TestDownloadOptions_Download(t *testing.T) {
182182
Uri: blobLoc.String(),
183183
Metadata: &core.BlobMetadata{
184184
Type: &core.BlobType{
185-
Dimensionality: core.BlobType_SINGLE,
186-
Format: "xyz",
187-
FileExtension: "xyz",
188-
EnableLegacyFilename: false,
185+
Dimensionality: core.BlobType_SINGLE,
186+
Format: "xyz",
187+
FileExtension: "xyz",
189188
},
190189
},
191190
},
192191
},
193192
},
194193
}},
195-
"legacy_blob": {Value: &core.Literal_Scalar{
194+
"csv_blob": {Value: &core.Literal_Scalar{
196195
Scalar: &core.Scalar{
197196
Value: &core.Scalar_Blob{
198197
Blob: &core.Blob{
199198
Uri: blobLoc.String(),
200199
Metadata: &core.BlobMetadata{
201200
Type: &core.BlobType{
202-
Dimensionality: core.BlobType_SINGLE,
203-
Format: "xyz",
204-
FileExtension: "xyz",
205-
EnableLegacyFilename: true,
201+
Dimensionality: core.BlobType_SINGLE,
202+
Format: "xyz",
203+
FileExtension: "xyz",
206204
},
207205
},
208206
},
@@ -212,7 +210,7 @@ func TestDownloadOptions_Download(t *testing.T) {
212210
},
213211
}))
214212
assert.NoError(t, dopts.Download(ctx), "Download Operation failed")
215-
assert.ElementsMatch(t, []string{"inputs.json", "inputs.pb", "x", "y", "blob.xyz", "legacy_blob", "legacy_blob.xyz"}, collectFile(tmpDir))
213+
assert.ElementsMatch(t, []string{"inputs.json", "inputs.pb", "x", "y", "blob.xyz", "csv_blob.csv"}, collectFile(tmpDir))
216214
})
217215

218216
t.Run("primitiveAndMissingBlobInputs", func(t *testing.T) {

flytecopilot/data/download.go

Lines changed: 22 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -33,34 +33,28 @@ type Downloader struct {
3333
mode core.IOStrategy_DownloadMode
3434
}
3535

36-
// By default, blobs (FlyteFiles) were not and still are not written with a file extension.
37-
// For example, a data: FlyteFile["csv"] blob should be written to "data", even though
38-
// Format="csv".
36+
// By default, blobs (FlyteFiles) were not and still are not written with a
37+
// file extension. For example, a data: FlyteFile["csv"] blob is written
38+
// to "inputs/data", even though Format="csv".
3939
//
4040
// When FileExtension="" (the default), this old behavior is preserved.
4141
//
42-
// However, a data: Annotated[FlyteFile["csv"], FileDownloadConfig(file_extension="csv")]
43-
// blob should be written to "data.csv" - both Format="csv" and FileExtension="csv" (new behavior).
44-
//
45-
// So when e.g. FileExtension="csv", the file is written to "data.csv".
46-
// Also, when e.g. FileExtension="csv" and EnableLegacyFilename=true, the file is written to
47-
// "data" and "data.csv" (partially new behavior, bridges the gap of backward compatibility).
48-
func resolveVarFilenames(vars *core.VariableMap) map[string][]string {
49-
varFilenames := make(map[string][]string, len(vars.GetVariables()))
42+
// However, an input blob
43+
// `data: Annotated[FlyteFile["csv"], FileDownloadConfig(file_extension="csv")]`
44+
// should be written to "inputs/data.csv" (when FileExtension="csv" - new behavior).
45+
func resolveVarFilenames(vars *core.VariableMap) map[string]string {
46+
varFilenames := make(map[string]string, len(vars.GetVariables()))
5047
for varName, variable := range vars.GetVariables() {
5148
varType := variable.GetType()
5249
switch varType.GetType().(type) {
5350
case *core.LiteralType_Blob:
5451
if varType.GetBlob().GetFileExtension() == "" {
55-
varFilenames[varName] = append(varFilenames[varName], varName)
52+
varFilenames[varName] = varName
5653
} else {
57-
varFilenames[varName] = append(varFilenames[varName], varName+"."+varType.GetBlob().GetFileExtension())
58-
if varType.GetBlob().GetEnableLegacyFilename() {
59-
varFilenames[varName] = append(varFilenames[varName], varName)
60-
}
54+
varFilenames[varName] = varName + "." + varType.GetBlob().GetFileExtension()
6155
}
6256
default:
63-
varFilenames[varName] = append(varFilenames[varName], varName)
57+
varFilenames[varName] = varName
6458
}
6559
}
6660
return varFilenames
@@ -465,7 +459,7 @@ func (d Downloader) handleLiteral(ctx context.Context, lit *core.Literal, filePa
465459
if err != nil {
466460
return nil, nil, errors.Wrapf(err, "failed to create directory [%s]", filePath)
467461
}
468-
v, m, err := d.RecursiveDownload(ctx, lit.GetMap(), filePath, make(map[string][]string), writeToFile)
462+
v, m, err := d.RecursiveDownload(ctx, lit.GetMap(), filePath, make(map[string]string), writeToFile)
469463
if err != nil {
470464
return nil, nil, err
471465
}
@@ -501,7 +495,7 @@ type downloadedResult struct {
501495
v interface{}
502496
}
503497

504-
func (d Downloader) RecursiveDownload(ctx context.Context, inputs *core.LiteralMap, dir string, varFilenames map[string][]string, writePrimitiveToFile bool) (VarMap, *core.LiteralMap, error) {
498+
func (d Downloader) RecursiveDownload(ctx context.Context, inputs *core.LiteralMap, dir string, varFilenames map[string]string, writePrimitiveToFile bool) (VarMap, *core.LiteralMap, error) {
505499
childCtx, cancel := context.WithCancel(ctx)
506500
defer cancel()
507501
if inputs == nil || len(inputs.GetLiterals()) == 0 {
@@ -519,26 +513,18 @@ func (d Downloader) RecursiveDownload(ctx context.Context, inputs *core.LiteralM
519513
}
520514
logger.Infof(ctx, "read object at location [%s]", offloadedMetadataURI)
521515
}
516+
filename := variable
517+
if varFilename, ok := varFilenames[variable]; ok {
518+
filename = varFilename
519+
}
520+
varPath := path.Join(dir, filename)
522521
lit := literal
523522
f[variable] = futures.NewAsyncFuture(childCtx, func(ctx2 context.Context) (interface{}, error) {
524-
var filenames []string
525-
var resultLit *core.Literal
526-
var resultV interface{}
527-
var err error
528-
if len(varFilenames[variable]) == 0 {
529-
filenames = []string{variable}
530-
} else {
531-
filenames = varFilenames[variable]
532-
}
533-
for _, filename := range filenames {
534-
varPath := path.Join(dir, filename)
535-
// TODO: Refactor handleLiteral to accept a list of file paths and return a list of downloaded results
536-
resultV, resultLit, err = d.handleLiteral(ctx2, lit, varPath, writePrimitiveToFile)
537-
if err != nil {
538-
return nil, err
539-
}
523+
v, lit, err := d.handleLiteral(ctx2, lit, varPath, writePrimitiveToFile)
524+
if err != nil {
525+
return nil, err
540526
}
541-
return downloadedResult{lit: resultLit, v: resultV}, nil
527+
return downloadedResult{lit: lit, v: v}, nil
542528
})
543529
}
544530

flytecopilot/data/download_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ func TestRecursiveDownload(t *testing.T) {
231231
}
232232
}()
233233

234-
varMap, lMap, err := d.RecursiveDownload(context.Background(), inputs, toPath, map[string][]string{}, true)
234+
varMap, lMap, err := d.RecursiveDownload(context.Background(), inputs, toPath, make(map[string]string), true)
235235
assert.NoError(t, err)
236236
assert.NotNil(t, varMap)
237237
assert.NotNil(t, lMap)
@@ -309,7 +309,7 @@ func TestRecursiveDownload(t *testing.T) {
309309
}
310310
}()
311311

312-
varMap, lMap, err := d.RecursiveDownload(context.Background(), inputs, toPath, map[string][]string{}, true)
312+
varMap, lMap, err := d.RecursiveDownload(context.Background(), inputs, toPath, make(map[string]string), true)
313313
assert.NoError(t, err)
314314
assert.NotNil(t, varMap)
315315
assert.NotNil(t, lMap)

flytecopilot/data/upload.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,11 +107,11 @@ func (u Uploader) handleBlobType(ctx context.Context, localPath string, toPath s
107107
}
108108
}
109109

110-
return coreutils.MakeLiteralForBlob(toPath, false, "", "", false), nil
110+
return coreutils.MakeLiteralForBlob(toPath, false, "", ""), nil
111111
}
112112
size := info.Size()
113113
// Should we make this a go routine as well, so that we can introduce timeouts
114-
return coreutils.MakeLiteralForBlob(toPath, false, "", "", false), UploadFileToStorage(ctx, fpath, toPath, size, u.store)
114+
return coreutils.MakeLiteralForBlob(toPath, false, "", ""), UploadFileToStorage(ctx, fpath, toPath, size, u.store)
115115
}
116116

117117
func (u Uploader) RecursiveUpload(ctx context.Context, vars *core.VariableMap, fromPath string, metaOutputPath, dataRawPath storage.DataReference) error {

flyteidl/clients/go/assets/admin.swagger.json

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

flyteidl/clients/go/coreutils/literals.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -485,7 +485,7 @@ func MakeLiteralForStructuredDataSet(path storage.DataReference, columns []*core
485485
}
486486
}
487487

488-
func MakeLiteralForBlob(path storage.DataReference, isDir bool, format string, fileExtension string, enableLegacyFilename bool) *core.Literal {
488+
func MakeLiteralForBlob(path storage.DataReference, isDir bool, format string, fileExtension string) *core.Literal {
489489
dim := core.BlobType_SINGLE
490490
if isDir {
491491
dim = core.BlobType_MULTIPART
@@ -501,7 +501,6 @@ func MakeLiteralForBlob(path storage.DataReference, isDir bool, format string, f
501501
Dimensionality: dim,
502502
Format: format,
503503
FileExtension: fileExtension,
504-
EnableLegacyFilename: enableLegacyFilename,
505504
},
506505
},
507506
},
@@ -603,7 +602,7 @@ func MakeLiteralForType(t *core.LiteralType, v interface{}) (*core.Literal, erro
603602

604603
case *core.LiteralType_Blob:
605604
isDir := newT.Blob.GetDimensionality() == core.BlobType_MULTIPART
606-
lv := MakeLiteralForBlob(storage.DataReference(fmt.Sprintf("%v", v)), isDir, newT.Blob.GetFormat(), newT.Blob.GetFileExtension(), newT.Blob.GetEnableLegacyFilename())
605+
lv := MakeLiteralForBlob(storage.DataReference(fmt.Sprintf("%v", v)), isDir, newT.Blob.GetFormat(), newT.Blob.GetFileExtension())
607606
return lv, nil
608607

609608
case *core.LiteralType_Schema:

flyteidl/clients/go/coreutils/literals_test.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -392,7 +392,6 @@ func TestMakeLiteralForBlob(t *testing.T) {
392392
isDir bool
393393
format string
394394
fileExtension string
395-
enableLegacyFilename bool
396395
}
397396
tests := []struct {
398397
name string
@@ -401,12 +400,11 @@ func TestMakeLiteralForBlob(t *testing.T) {
401400
}{
402401
{"simple-key", args{path: "/key", isDir: false, format: "xyz"}, &core.Blob{Uri: "/key", Metadata: &core.BlobMetadata{Type: &core.BlobType{Format: "xyz", Dimensionality: core.BlobType_SINGLE}}}},
403402
{"simple-dir", args{path: "/key", isDir: true, format: "xyz"}, &core.Blob{Uri: "/key", Metadata: &core.BlobMetadata{Type: &core.BlobType{Format: "xyz", Dimensionality: core.BlobType_MULTIPART}}}},
404-
{"simple-key-with-extension", args{path: "/key", isDir: false, format: "xyz", fileExtension: "xyz", enableLegacyFilename: false}, &core.Blob{Uri: "/key", Metadata: &core.BlobMetadata{Type: &core.BlobType{Format: "xyz", Dimensionality: core.BlobType_SINGLE, FileExtension: "xyz", EnableLegacyFilename: false}}}},
405-
{"simple-key-with-extension-and-legacy-filename", args{path: "/key", isDir: false, format: "xyz", fileExtension: "xyz", enableLegacyFilename: true}, &core.Blob{Uri: "/key", Metadata: &core.BlobMetadata{Type: &core.BlobType{Format: "xyz", Dimensionality: core.BlobType_SINGLE, FileExtension: "xyz", EnableLegacyFilename: true}}}},
403+
{"simple-key-with-extension", args{path: "/key", isDir: false, format: "xyz", fileExtension: "xyz"}, &core.Blob{Uri: "/key", Metadata: &core.BlobMetadata{Type: &core.BlobType{Format: "xyz", Dimensionality: core.BlobType_SINGLE, FileExtension: "xyz"}}}},
406404
}
407405
for _, tt := range tests {
408406
t.Run(tt.name, func(t *testing.T) {
409-
if got := MakeLiteralForBlob(tt.args.path, tt.args.isDir, tt.args.format, tt.args.fileExtension, tt.args.enableLegacyFilename); !reflect.DeepEqual(got.GetScalar().GetBlob(), tt.want) {
407+
if got := MakeLiteralForBlob(tt.args.path, tt.args.isDir, tt.args.format, tt.args.fileExtension); !reflect.DeepEqual(got.GetScalar().GetBlob(), tt.want) {
410408
t.Errorf("MakeLiteralForBlob() = %v, want %v", got, tt.want)
411409
}
412410
})

flyteidl/gen/pb-es/flyteidl/core/types_pb.ts

Lines changed: 1 addition & 12 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)