Skip to content
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

LocalEnvironment to stop being IDisposable #1287

Closed
Zruty0 opened this issue Oct 17, 2018 · 4 comments · Fixed by #2568
Closed

LocalEnvironment to stop being IDisposable #1287

Zruty0 opened this issue Oct 17, 2018 · 4 comments · Fixed by #2568
Assignees
Labels
API Issues pertaining the friendly API

Comments

@Zruty0
Copy link
Contributor

Zruty0 commented Oct 17, 2018

The only reason we have IHostEnvironment as an IDisposable is because we clean up the temp files created using CreateTempFile. This functionality is not really very useful. We should remove the functionality, remove the IDisposable and fix all the using calls in the codebase (preferably to just new MLContext, if possible)

@Zruty0 Zruty0 added the API Issues pertaining the friendly API label Oct 17, 2018
@TomFinley
Copy link
Contributor

The implication is that anything that relies on it for temporary file creation and that requires must cease to do so. In order for us to do this, we must also remove temporary file creation from IHostEnvironment. (Also, regular file creation, though that is less urgent.) This I have found untangling this relatively easy to do, but there is one exception that is very troublesome because it explicitly relies on this behavior.

// We are not disposing the fileHandle because we want it to stay around for the execution of the graph.
// It will be disposed when the environment is disposed.
var fileHandle = host.CreateTempFile();

That's bad, but I think there's a pretty easily solution. I think it is completely reasonable for someone asking for disk cache to provide a place where on the disk it should be stored. I think that will actually be an improvement -- certainly it is not an expectation that any "temporary" files will be many gigabytes or terabytes large (which is the only conceivable reason why anyone would use disk caching in the first place), so having some ability to configure where it is stored would I think be good. Or we provide access to the binary saver via entry-points and say that if someone wants that functionality they just have to do it themselves. This I think is fine, since entry-points were entry-points into ML.NET for wrapping code, not user facing code.

@glebuk
Copy link
Contributor

glebuk commented Jan 7, 2019

@TomFinley is this still relevant? IHostEnvironment is no longer disposable AFAIK.

@eerhardt
Copy link
Member

Yes, this is still relevant.

We still have these methods on IHostEnvironment that we need to remove:

/// <summary>
/// Return a file handle for an input "file".
/// </summary>
IFileHandle OpenInputFile(string path);
/// <summary>
/// Create an output "file" and return a handle to it.
/// </summary>
IFileHandle CreateOutputFile(string path);
/// <summary>
/// Create a temporary "file" and return a handle to it. Generally temp files are expected to be
/// written to exactly once, and then can be read multiple times.
/// Note that IFileHandle derives from IDisposable. Clients may dispose the IFileHandle when it is
/// no longer needed, but they are not required to. The host environment should track all temp file
/// handles and ensure that they are disposed properly when the environment is "shut down".
///
/// The suffix and prefix are optional. A common use for suffix is to specify an extension, eg, ".txt".
/// The use of suffix and prefix, including whether they have any affect, is up to the host environment.
/// </summary>
[Obsolete("The host environment is not disposable, so it is inappropriate to use this method. " +
"Please handle your own temporary files within the component yourself, including their proper disposal and deletion.")]
IFileHandle CreateTempFile(string suffix = null, string prefix = null);

Even though it doesn't implement IDisposable, we can't have these methods on this interface.

@eerhardt
Copy link
Member

Just an FYI - it looks like the only place in the product that is calling Cache.CacheData (which is the only place calling CreateTempFile pointed out by @TomFinley above) is coming from Entry-Points:

var cacheView = Cache.CacheData(host, new Cache.CacheInput()
{
Data = roleMappedData.Data,
Caching = cachingType.Value
}).OutputData;

This is good, because it means you can't get to this code from public APIs. And we can force the EntryPoints to call Dispose to clean up the temp files.

maryamariyan added a commit to maryamariyan/machinelearning that referenced this issue Feb 14, 2019
eerhardt pushed a commit that referenced this issue Feb 15, 2019
…nt (#2539)

* Remove OpenInputFile and CreateOutputFile from IHostEnvironment

Related to: #1287

* Add missing corner case validation
maryamariyan added a commit to maryamariyan/machinelearning that referenced this issue Feb 15, 2019
maryamariyan added a commit to maryamariyan/machinelearning that referenced this issue Feb 15, 2019
maryamariyan added a commit that referenced this issue Feb 27, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Issues pertaining the friendly API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants