fix: bug: memory leak when using bentoml>=1.2

[frostming]

Fixes #4760

Signed-off-by: Frost Ming me@frostming.com

What does this PR address?

Change the request temp dir to create on demand.
/cc @Zheaoli @gusghrlrl101

[frostming]

Do you think we need to reuse the temp directories across different requests, i.e. to use a temp dir pool. Is that overkill?

[Zheaoli]

Do you think we need to reuse the temp directories across different requests, i.e. to use a temp dir pool. Is that overkill?

It's not recommended, I think.

I prefer to make a tmp directory when it's usable like it's a file style request. Rather than that, I think it would be waste to make a tmp directory for every request

[Zheaoli]

I think maybe we don't need a directory per request? We can group by the directory by thread/process id, and all the request file in the same directory?

[frostming]

I prefer to make a tmp directory when it's usable like it's a file style request. Rather than that, I think it would be waste to make a tmp directory for every request

Yes, that is exactly what this PR does. The creation is lazy.

[Zheaoli]

Yes, that is exactly what this PR does. The creation is lazy.

Yep, I got it. But I might have another idea here

I think maybe we don't need a directory per request? We can group by the directory by thread/process id, and all the request file in the same directory?

[frostming]

I think maybe we don't need a directory per request? We can group by the directory by thread/process id, and all the request file in the same directory?

But we would also need to clean it periodically or its size will keep growing. The purpose of the temp dir is to isolate the store path for each request which is handled in coroutines, so using a process id/thread id wouldn't work. The same process and thread may be handling more than one request concurrently. For example, it makes the following work:

@bentoml.api
def predict(self, input: int, ctx: bentoml.Context) -> Path:
    output_path = Path(ctx.temp_dir, "output.txt")  # this file shouldn't be overwritten by other request
    with open(output_path, 'wb') as f:
        f.write(some_bytes)
    return output_path

[Zheaoli]

I think maybe we don't need a directory per request? We can group by the directory by thread/process id, and all the request file in the same directory?

But we would also need to clean it periodically or its size will keep growing. The purpose of the temp dir is to isolate the store path for each request which is handled in coroutines, so using a process id/thread id wouldn't work. The same process and thread may be handling more than one request concurrently. For example, it makes the following work:

@bentoml.api

def predict(self, input: int, ctx: bentoml.Context) -> Path:

    output_path = Path(ctx.temp_dir, "output.txt")  # this file shouldn't be overwritten by other request

    with open(output_path, 'wb') as f:

        f.write(some_bytes)

    return output_path

oh I got your means, you want to the developer following the common conventions like all files during request in the same directory

[Zheaoli]

LGTM

[xianml]

How about we just use a mem file instead? say pyfakefs or just use fssepc to create a memory file system.
After all, we do not really need to sink data to real file. right?

[frostming]

How about we just use a mem file instead

That would be better, but our goal is to provide a seamless experience so users can save files using the familiar os.path or pathlib.Path interfaces, neither one of the above does.

[xianml]

That would be better, but our goal is to provide a seamless experience so users can save files using the familiar os.path or pathlib.Path interfaces, neither one of the above does.

mem file is also an impl of a file interface. And use it as the impl of ctx.temp_dir, can be returned as a Path.
is it feasible ?

[frostming]

mem file is also an impl of a file interface. And use it as the impl of ctx.temp_dir, can be returned as a Path.
is it feasible ?

I am afraid not, many inference functions expect a string as the input or output path. And we will lose the memfile magic.

[aarnphm]

hmm can we use memfile and still offer pathlib-like API here?

I guess that FS API is actually not very well known afaict

[frostming]

hmm can we use memfile and still offer pathlib-like API here?

but os.path apis won't work right? That's extra learning burden for users.