Allow a file to be moved cross-device.

[miku]

Description

Implementation of luigi.File would now allow movement of a file across filesystems.

Motivation and Context

If you use luigi.File(src).move(target) when source and target are on different filesystems, you get an error:

OSError: [Errno 18] Invalid cross-device link

Also, this would align implementation of File.move() with File.copy() which already uses shutil.

Have you tested this? If so, how?

Semantic stays the same. Existing tests apply.

If the destination is on the current filesystem, then os.rename() is used. - shutil.move

Important tradeoff: If you move a file across filesystems, you lose atomicity of os.rename.

[mention-bot]

@miku, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Tarrasch, @erikbern and @freider to be potential reviewers.

[erikbern]

lgtm

[Tarrasch]

Can you add the following?

• Tests actually moving across fs's, or at least mock out that shutil.move is called
• Mention the atomicity things in the docs? Atomicity of renames are a very important thing in luigi. If it can't be maintained for some functions, the docs should say under what condition.

Other than that, LGTM

[miku]

Thanks for the review, @Tarrasch, I tried to address your points. As for the docs, I didn't find a perfect place yet, so added a short section to the tasks docs.

[erikbern]

Mention the atomicity things in the docs? Atomicity of renames are a very important thing in luigi. If it can't be maintained for some functions, the docs should say under what condition.

pretty sure cross device file moving will never be atomic

[miku]

@erikbern, closest thing to atomic would probably be: copy file to a temp location on the target device, rename it to destination, then unlink the source? But I wanted to keep the code short.

Luigi allows non-atomicity, e.g. in tasks with multiple outputs:

If multiple outputs are returned, atomicity will be lost ... -- Task.output

So I thought it would be ok to drop atomicity if this special case, if the user chooses to (chooses to work with two filesystems).

[Tarrasch]

@erikbern Althogh the luigi docs shouldn't go into details about things a savy engineer might have already inferred. All its readers are still humans and many might benefit from a warning-bubble like the one I'm suggesting.

[Tarrasch]

Hmm, can't you do the much more elegant context-manager thingie? with-statements.

[miku]

Yes, sure. Almost done.

[Tarrasch]

Wouldn't it be much more easy to put this in the docs for LocalTarget.move() docs?? You don't even need a code example. Just a warning saying to be aware of the risks when doing this across filesystems's.

[miku]

Yes, you're right. I addressed this the next PR.

[erikbern]

@erikbern, closest thing to atomic would probably be: copy file to a temp location on the target device, rename it to destination, then unlink the source? But I wanted to keep the code short.

great idea, let's do this

[miku]

great idea, let's do this

Thanks for the feedback. I implemented an almost-atomic cross-device move operation and updated the docs. (I'll be AFK for ten days, can only look into this when I'm back).

[Tarrasch]

Does this work for you in production as well? If so, let's merge. :)

[erikbern]

lgtm – nice!

[miku]

Does this work for you in production as well?

@Tarrasch, yes it does.

Background: We have $TEMPDIR and data on different partitions plus we sometimes do writes to a tempfile first, then manually move the file into place with a

luigi.LocalTarget(sometempfile).move(self.output().path)

[miku]

Thanks!