Luigi: Temporary files are messed up

[erikbern]

Luigi has like 500 different inconsistent implementations of temporary files. We should clean this up.

There's lots of cases where you need temporary files. In most of those cases, you want some kind of action on close() or using a context manager. For instance if you are working with a LocalTarget, you want to be able to do both

f = self.output().open('w')
# ...
f.close() # this commits the file

As well as

with self.output().open('w') as f:
    # ...

But there are also cases where you want access to the underlying filename, in which case I suggest something like this (this was originally @Tarrasch's idea)

with self.output().temporary_path() as fn:
   h5py.open(fn) # or some stuff like that...

Do we need semantics here that does not involve a context manager? In that case you could probably do something that returns a _new_ LocalTarget, such as

t = self.output().temporary_target()
# print t.path
t.commit()

I'm not sure if this is necessary.

There's also a bunch of other targets that leverage temp files, like writing to a file over FTP (RemoteTarget). This leverages a local file as a temporary output. It would be great if RemoteTarget could simply inherit all the methods above so that you have both semantics for atomic stream opens, as well as getting an explicit local path. And you should be able to do it using a context manager or not.

Finally there's code in Luigi that just uses temporary paths in general. Running a Python mapreduce job creates a temporary directory. We should be able to leverage the internal temporary file implementation here.

[Tarrasch]

This is absolutely correct. I'll doubt I'll be able to work on this myself until early March though. Thanks for creating this ticket for this recurring issue that we have.

[erikbern]

If you can leave some notes here, I would appreciate it. I wanted to spend a couple of hours yesterday refactoring this but realized it was a bigger project just to understand the code. So even just outlining a strategy for refactoring would be great

[Tarrasch]

Here are two ideas of what one can do that simplifies the code base and is taking us in the right direction.

1. Remove is_tmp= kwarg to targets.

Currently we have a "convenience" kwarg is_tmp. Works like this:

    t = luigi.LocalTarget(is_tmp=True)
    args = ['python', 'test/cmdline_test.py', 'WriteToFile', '--filename', t.path, '--local-scheduler', '--no-lock']
    self._run_cmdline(args)

See how convenient it is that you don't even need to supply a path? :)

Now why I don't like this: In luigi we encourage usage of targets like this:

class MyTask(luigi.Task):
    def output(self): return LocalTarget(...)
    def run(self): do_something(self.output().path); 

But the is_tmp thing is directly breaking the MyTarget(*args, **kwargs) == MyTarget(*args, **kwargs) invariant.

2. Unify -luigi-tmp-%09d' % random.randrange(0, 1e10) implementations:

It's used here:

~/vng/repos/luigi master  
❯ ag luigi-tmp
luigi/contrib/hadoop_jar.py
49:                y = luigi.contrib.hdfs.HdfsTarget(x_path_no_slash + '-luigi-tmp-%09d' % random.randrange(0, 1e10))

luigi/contrib/ftp.py
169:            tmp_path = folder + os.sep + 'luigi-tmp-%09d' % random.randrange(0, 1e10)
187:        tmp_local_path = local_path + '-luigi-tmp-%09d' % random.randrange(0, 1e10)
261:            self.__tmp_path = self.path + '-luigi-tmp-%09d' % random.randrange(0, 1e10)

luigi/contrib/ssh.py
257:        tmp_path = path + '-luigi-tmp-%09d' % random.randrange(0, 1e10)
271:        tmp_local_path = local_path + '-luigi-tmp-%09d' % random.randrange(0, 1e10)
288:        self.__tmp_path = self.path + '-luigi-tmp-%09d' % random.randrange(0, 1e10)

luigi/file.py
42:        return path + '-luigi-tmp-%09d' % random.randrange(0, 1e10)
105:            path = os.path.join(tempfile.gettempdir(), 'luigi-tmp-%09d' % random.randint(0, 999999999))
147:        tmp = LocalTarget(new_path + '-luigi-tmp-%09d' % random.randrange(0, 1e10), is_tmp=True)

I think it also should use .format() like this '-luigi-tmp-{:010}'.format(random.randrange(0, 1e10))

But a more complete implementation already exists for hdfs. In particular it adds an optional <username> and can handle input that look like this hdfs://123.456.0.1/my/path/to/file.txt?that=has,query=args

[erikbern]

sounds good – we can start with those two and then keep refactoring. thanks!

[PeteW]

The BaseHadoopJobTask doesnt seem to respect the output convention.
When I run a HiveQueryTask no file is touched when the task is successful.
You need to have like a self.output().open("w").close() somewhere within BaseHadoopJobTask or higher up, but I'm not certain where to place it. The code in git is different than the one in my environment, so I'm going to add it to my local copy and hope for the best

[imbilltucker]

I want a context manager that uses something like NamedTemporaryFile and then finalizes it to the self.output().fn if successful.

[Tarrasch]

@imbilltucker, here is some code I've been using in production now. Haven't had time to make a clean PR out of it. Do you want to take a stab?

Definition

class VngHdfsTarget(luigi.contrib.hdfs.HdfsTarget):
    def temporary_path(target):
        class Manager(object):
            def __enter__(self):
                path = target.path.rstrip('/')
                num = random.randrange(0, 1e10)
                self.temp_path = path + '-luigi-tmp-{:010}'.format(num)
                return self.temp_path

            def __exit__(self, exc_type, exc_value, traceback):
                if exc_type is None:
                    # There were no exceptions
                    target.fs.rename_dont_move(self.temp_path, target.path)
                return False

        return Manager()

Usage

    def run(self):
        with self.output().temporary_path() as self.temp_output_path:
            run_some_external_command(output_dir=self.temp_output_path) 

---

The task would be to copy-paste the temporary_path to the FileSystemTarget or something pretty high up in the inheritance hierarchy. Add tests, make other places in code base use it, document it and eventually deprecate the is_tmp functionality.

[sbliven]

Can we get a temporary directory version of this? I have an external job that creates a directory structure, and I want to ensure atomicity.

[Tarrasch]

@sbliven have you tried the temporary_path decorator introduced since #1909, it works on directories too. In fact that's the "typical" case (like running a spark or MR job).

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If closed, you may revisit when your time allows and reopen! Thank you for your contributions.