Temporary files are never cleaned

[vincbon]

While doing cross-validation for hyperparameters tuning, I noticed the filesystem usage growing indefinitely.
During processing some directories are created in /tmp, but they are never cleaned.

Looking at the code I think the issue is in the fit method in models.py:

def fit(self, stan_init, stan_data, **kwargs):
        (stan_init, stan_data) = self.prepare_data(stan_init, stan_data)

        if 'inits' not in kwargs and 'init' in kwargs:
            kwargs['inits'] = self.prepare_data(kwargs['init'], stan_data)[0]

        args = dict(
            data=stan_data,
            inits=stan_init,
            algorithm='Newton' if stan_data['T'] < 100 else 'LBFGS',
            iter=int(1e4),
            output_dir = mkdtemp(),
        )
        args.update(kwargs)

        try:
            self.stan_fit = self.model.optimize(**args)

The mkdtemp function requires the user to manually delete the directory once it is no longer needed (see the doc) but I don't see where it is cleaned here.

The cmdstanpy doc also says the following:

:param output_dir: Name of the directory to which CmdStan output
            files are written. If unspecified, output files will be written
            to a temporary directory which is deleted upon session exit.

So I'm wondering if output_dir = mkdtemp() is really necessary here ?

Thank you!

[vincbon]

I just stumbled upon #2088. This line was added to avoid concurrency issues so it is indeed needed.
Like in #2160 TemporaryDirectory should be preferred to handle the cleaning process automatically.

[WardBrian]

Since stan-dev/cmdstanpy#569 (e.g. cmdstanpy 1.0.2+) cmdstanpy itself should avoid this problem. #2088 was before this release, when all cmdstan runs were saved in one temp folder, which could lead to collisions.

[tcuongd]

Just double checking:

1. cmdstanpy >= 1.0.2 would automatically create a different temp folder for each invocation of fit(), so that takes care of collision issues (different processes writing to the same file).
2. However, cleanup is another issue. mkdtemp(), which is used currently in prophet's code and cmdstanpy's code, doesn't handle cleanup automatically, and we prefer TemporaryDirectory() instead. @WardBrian what do you think about making that change in cmdstanpy? Otherwise we can just change it in the Prophet code.

[WardBrian]

Cmdstanpy cleans up its own files using atexit

[tcuongd]

Cmdstanpy cleans up its own files using atexit

Yep nice, just tested and it's working as expected for CV. Will do a quick PR to bump the minimum version for cmdstanpy.