Skip to content

Allow passing "delete=False" to TemporaryDirectory #69212

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

Closed
anntzer mannequin opened this issue Sep 8, 2015 · 23 comments
Closed

Allow passing "delete=False" to TemporaryDirectory #69212

anntzer mannequin opened this issue Sep 8, 2015 · 23 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@anntzer
Copy link
Mannequin

anntzer mannequin commented Sep 8, 2015

BPO 25024
Nosy @birkenfeld, @ncoghlan, @pitrou, @bitdancer, @serhiy-storchaka, @maurorodrigues, @asottile, @epicfaace, @CAM-Gerlach, @septatrix, @nyuszika7h, @beliaev-maksim
PRs
  • bpo-29982: Add "ignore_cleanup_errors" param to tempfile.TemporaryDirectory() #24793
  • Files
  • TemporaryDirectory_delete_false.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2020-03-26.19:32:11.879>
    created_at = <Date 2015-09-08.05:36:20.331>
    labels = ['type-feature', 'library']
    title = 'Allow passing "delete=False" to TemporaryDirectory'
    updated_at = <Date 2021-11-02.10:35:39.100>
    user = 'https://github.com/anntzer'

    bugs.python.org fields:

    activity = <Date 2021-11-02.10:35:39.100>
    actor = 'beliaev-maksim'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-03-26.19:32:11.879>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2015-09-08.05:36:20.331>
    creator = 'Antony.Lee'
    dependencies = []
    files = ['40694']
    hgrepos = []
    issue_num = 25024
    keywords = ['patch']
    message_count = 22.0
    messages = ['250158', '250217', '250386', '250387', '252375', '364345', '364453', '364458', '364461', '364466', '364468', '365103', '365105', '365106', '365107', '365108', '365109', '382595', '387785', '388220', '394462', '405499']
    nosy_count = 12.0
    nosy_names = ['georg.brandl', 'ncoghlan', 'pitrou', 'r.david.murray', 'serhiy.storchaka', 'maurosr', 'Anthony Sottile', 'epicfaace', 'CAM-Gerlach', 'Nils Kattenbeck', 'nyuszika7h', 'beliaev-maksim']
    pr_nums = ['24793']
    priority = 'normal'
    resolution = 'rejected'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue25024'
    versions = ['Python 3.6']

    @anntzer
    Copy link
    Mannequin Author

    anntzer mannequin commented Sep 8, 2015

    I would like to suggest allowing passing "delete=False" to the TemporaryDirectory constructor, with the effect that the directory is not deleted when the TemporaryDirectory context manager exits, or when the TemporaryDirectory object is deleted.

    I realize that this would effectively duplicate the functionality of mkdtemp, but this is similar to the redundancy between NamedTemporaryFile(delete=False) and mkstemp, and would make it easier to switch between the two behaviors (which is useful e.g. when debugging, where you may need to look at the temporary files "post-mortem").

    @anntzer anntzer mannequin added the stdlib Python modules in the Lib dir label Sep 8, 2015
    @bitdancer
    Copy link
    Member

    The two cases are not parallel, in that NamedTemporaryFile closes the file at the end of the context manager, whereas the *only* thing the TemporaryDirectory does is delete the directory on cleanup.

    There is some value to the "parallel interface" argument, so I'm only -0 on this.

    @bitdancer bitdancer added the type-feature A feature request or enhancement label Sep 8, 2015
    @serhiy-storchaka
    Copy link
    Member

    I'm inclined to reject this idea too.

    @pitrou
    Copy link
    Member

    pitrou commented Sep 10, 2015

    The idea looks ok to me, but you'd have to provide a patch.

    @maurorodrigues
    Copy link
    Mannequin

    maurorodrigues mannequin commented Oct 6, 2015

    Hi everybody!
    This is my second patch on the community, although the first one is not merged, so any feedback is appreciated.

    I've added tests to cover this new situation and docs to let people know about the possibility of keeping their temporary directories for debugging purposes.

    Regarding the code itself, I've also made a 'design decision' to remove the directory even when created with delete=False if cleanup method is called explicitly, so I would like to hear your thoughts on that matter specially if you don't agree with it.

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Mar 16, 2020

    I would certainly like to see this, it would eliminate my last few hand rolled temporary directory contexts

    Mauro would you be interested in re-posting this patch as a PR to github? (or allowing someone else to carry your patch forward?)

    @maurorodrigues
    Copy link
    Mannequin

    maurorodrigues mannequin commented Mar 17, 2020

    Hi Anthony,

    Thanks for asking, yeah I'm interested in push a new version. I'll do it later today and I'll post a link to the pr here.

    @serhiy-storchaka
    Copy link
    Member

    What is your use case Anthony?

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Mar 17, 2020

    one example is here: https://github.com/pre-commit/pre-commit/blob/bb6f1efe63c168d9393d520bd60e16c991a57059/pre_commit/store.py#L137-L139

    where I would want cleanup in the exceptional case

    another (related but different) closed-source use case involves creating a temporary directory that may be deleted and currently has to be guarded by:

        shutil.rmtree(..., ignore_errors=True)

    (making TemporaryDirectory not useful since it crashes if the directory goes missing)

    there's additionally the problem of readonly files on windows, and readonly directories elsewhere being undeletable without a custom rmtree but I think that's a different issue entirely -- https://github.com/pre-commit/pre-commit/blob/bb6f1efe63c168d9393d520bd60e16c991a57059/pre_commit/util.py#L250-L267

    @serhiy-storchaka
    Copy link
    Member

    If you want the conditional cleanup, then TemporaryDirectory is obviously a wrong tool. mkdtemp looks more appropriate.

    If you remove directory in process of working with temporary directory, would it help if you create a subdirectory in the temporary directory and work with it (including conditionally removing)?

    As for the readonly files, was not this problem solved by PR 10320?

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Mar 17, 2020

    oh! that's neat, yeah hadn't been following closely enough it seems, good to hear that the readonly thing is fixed!

    @maurorodrigues
    Copy link
    Mannequin

    maurorodrigues mannequin commented Mar 26, 2020

    So per Serhiy comment can I assume the patch is not necessary? If so I believe the issue should be closed as well.

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Mar 26, 2020

    Serhiy's comment provides workarounds but are still (imo) inferior to supporting this.

    I would really like for this api to match that of NamedTemporaryFile which has the same delete=... option

    @serhiy-storchaka
    Copy link
    Member

    Could you please explain the difference between hypothetical TemporaryDirectory(delete=False) and simple mkdtemp()?

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Mar 26, 2020

    the differences are api compatibility with context manager protocol and equivalence with NamedTemporaryFile

    NamedTemporaryFile(delete=False) is just mkstemp afaict and the api is there

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Mar 26, 2020

    you are right though, the effect is the same as just using mkdtemp

    @serhiy-storchaka
    Copy link
    Member

    Well, then I think there is nothing to do this.

    Compatibility with context manager protocol is not a goal if it does nothing on exit. In any case you can easy to make it in three lines:

    @contextmanager
    def mymkdtemp():
        yield mkdtemp()

    NamedTemporaryFile is more complex than mkstemp(). It creates a file object and closes a file descriptor even if leave the file on the filesystem.

    @nyuszika7h
    Copy link
    Mannequin

    nyuszika7h mannequin commented Dec 6, 2020

    Sorry for reviving a 9 months old issue, but IMO there was no good reason to reject this especially when a patch was provided. Even if the context manager can be replaced with 3 lines of code, I still don't consider that very user-friendly.

    My use case would be passing delete=False temporarily while debugging my script, it would be much simpler than using a whole different hacky method when the end goal is to change it back to delete=True once it is completed anyway.

    What issues exactly does the addition of a simple option cause? I don't think something as trivial as this causes a maintenance burden, and you can't call it feature creep either when TemporaryDirectory doesn't have *any* other optional keyword arguments.

    @epicfaace
    Copy link
    Mannequin

    epicfaace mannequin commented Feb 27, 2021

    I agree -- as a user, it wasn't clear to me from looking at the documentation that mkdtemp was the right way to go to not delete directories. I had expected that NamedTemporaryDirectory would also support delete=False, just like NamedTemporaryFile.

    Given that we say in the documentation that "mkstemp() and mkdtemp() are lower-level functions which require manual cleanup", it seems to make sense to allow Python users who don't care / know about mkstemp / mkdtemp to just use the more user-friendly NamedTemporaryDirectory / NamedTemporaryFile classes. This would make the language more accessible.

    Would we be fine with reopening this issue so someone can contribute a patch?

    @CAM-Gerlach
    Copy link
    Member

    To note, the proposal on BPO-29982 should hopefully address one of the two use-cases described by Anthony Sotittle, ignore_errors=True, in a cleaner fashion that takes advantage of the existing higher-level functionality rather than duplicating mkdtemp. Opinions and feedback welcome over there.

    @Septatrix
    Copy link
    Mannequin

    Septatrix mannequin commented May 26, 2021

    While the proposal linked by C.A.M. solves one of the use cases but it does not address the others.

    One use cases which is rather common for me it is e.g. to have scripts/programs which allow configuring whether temporary directories should get deleted or stay persistent after program exit.
    Currently this always requires hand rolled wrappers like the following:

    def mkdtemp_persistent(*args, persistent=True, **kwargs):
        if persistent:
            @contextmanager
            def normal_mkdtemp():
                yield tempfile.mkdtemp()
            return normal_mkdtemp(*args, **kwargs)
        else:
            return tempfile.TemporaryDirectory(*args, **kwargs)
    
    with mkdtemp_persistent(persistent=False) as dir:
        ...

    Which gets the job done but is not as user friendly as other parts of the stdlib.

    @beliaev-maksim
    Copy link
    Mannequin

    beliaev-maksim mannequin commented Nov 2, 2021

    just want to correct code that Nils post in the last comment to really take args and kwargs

    def mkdtemp_persistent(*args, persistent=True, \*\*kwargs):
        if persistent:
            @contextmanager
            def normal_mkdtemp():
                yield tempfile.mkdtemp(*args, \*\*kwargs)
            return normal_mkdtemp()
        else:
            return tempfile.TemporaryDirectory(*args, \*\*kwargs)
    
    ```py
    with mkdtemp_persistent(persistent=False) as dir:
        ...
    

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @CAM-Gerlach
    Copy link
    Member

    CAM-Gerlach commented Mar 19, 2023

    Changing the close reason to reflect the previous reality; I also note that this was brought up again in #100131 with a corresponding PR, which FWIW I'm now supportive of. Perhaps worth re-opening and closing this one in #100132 , too.

    @CAM-Gerlach CAM-Gerlach closed this as not planned Won't fix, can't repro, duplicate, stale Mar 19, 2023
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants