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

[stdlib] Implement NamedTemporaryFile in tempfile module #2762

Closed
wants to merge 6 commits into from

Conversation

artemiogr97
Copy link
Contributor

@artemiogr97 artemiogr97 commented May 20, 2024

Implementation of NamedTemporaryFile a wrapper around a FileHandle that can be used as a context manager that is deleted after closing it if needed.

@artemiogr97
Copy link
Contributor Author

artemiogr97 commented May 20, 2024

@JoeLoser, @laszlokindrat, I'm having some issues with this:
The implementation that I had in #2352 used to work in an older release, in the current release I had the following compiler error:

for _ in range(TMP_MAX):  #error here: field 'self._file_handle' destroyed out of the middle of a value, preventing the overall value from being destroyed
    var potential_name = final_dir / (
        prefix + _get_random_name() + suffix
    )
    if os.path.exists(potential_name):
        continue
    try:
        self.name = potential_name
        self._file_handle = FileHandle(potential_name, mode=mode)
        return
    except:
        continue

I had to put the if condition inside the for to be able to compile:

for _ in range(TMP_MAX):
            var potential_name = final_dir / (
                prefix + _get_random_name() + suffix
            )
            try:
                if os.path.exists(potential_name) == False:
                    self.name = potential_name
                    self._file_handle = FileHandle(potential_name, mode=mode)
                    return
            except:
                continue

But now there is a runtime error which I don't really understand, I also had the same tests on my old PR and everything was ok.

@laszlokindrat
Copy link
Contributor

@artemiogr97 I think this is unblocked now. Are you still planning to work on this?

@artemiogr97
Copy link
Contributor Author

@laszlokindrat yes, I was planning to check again this week

@laszlokindrat
Copy link
Contributor

@laszlokindrat yes, I was planning to check again this week

Awesome, thanks, please ping me when you have something ready for review.

@artemiogr97 artemiogr97 marked this pull request as ready for review June 4, 2024 21:00
@artemiogr97 artemiogr97 requested a review from a team as a code owner June 4, 2024 21:00
@artemiogr97
Copy link
Contributor Author

@JoeLoser, @laszlokindrat, I'm having some issues with this: The implementation that I had in #2352 used to work in an older release, in the current release I had the following compiler error:

for _ in range(TMP_MAX):  #error here: field 'self._file_handle' destroyed out of the middle of a value, preventing the overall value from being destroyed
    var potential_name = final_dir / (
        prefix + _get_random_name() + suffix
    )
    if os.path.exists(potential_name):
        continue
    try:
        self.name = potential_name
        self._file_handle = FileHandle(potential_name, mode=mode)
        return
    except:
        continue

I had to put the if condition inside the for to be able to compile:

for _ in range(TMP_MAX):
            var potential_name = final_dir / (
                prefix + _get_random_name() + suffix
            )
            try:
                if os.path.exists(potential_name) == False:
                    self.name = potential_name
                    self._file_handle = FileHandle(potential_name, mode=mode)
                    return
            except:
                continue

But now there is a runtime error which I don't really understand, I also had the same tests on my old PR and everything was ok.

Turns out that adding an else after the for loop makes the compiler happier, style-wise I prefer the first implementation that I had so I changed it again

Copy link
Contributor

@laszlokindrat laszlokindrat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you! I mostly have nits and questions. Could you please add a changelog entry, and update the PR title and description with a summary of the patch?

stdlib/src/tempfile/tempfile.mojo Show resolved Hide resolved
stdlib/test/tempfile/test_tempfile.mojo Outdated Show resolved Hide resolved
self.name = existing.name^

@always_inline
fn read(self, size: Int64 = -1) raises -> String:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Just to confirm, providing these wrappers is necessary because we need to allow this to be used both within a context manager and as a normal object, right? (And also because in python this is actually a function).

This is fine, but I wonder if we could introduce a trait for things that implement these FileHandle-like interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I don't know if there is a better way to do this in mojo for now, just as a reference the python implementation does not wraps all the methods of the underlying object, it reimplements __getattr__ to directly return the methods of the real file object: https://github.com/python/cpython/blob/e83ce850f433fd8bbf8ff4e8d7649b942639db31/Lib/tempfile.py#L489-L506

I guess it would make sense to define some traits for IO stuff, the equivalent of the classes defined here: https://docs.python.org/3/library/io.html

stdlib/src/tempfile/tempfile.mojo Outdated Show resolved Hide resolved
@artemiogr97 artemiogr97 changed the title [stdlib] NamedTemporaryFile [stdlib] Implement NamedTemporaryFile in tempfile module Jun 5, 2024
Copy link
Collaborator

@JoeLoser JoeLoser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thank you for working on this! @Dan13llljws is excited to use this new functionality soon 😄

@JoeLoser
Copy link
Collaborator

JoeLoser commented Jun 6, 2024

!sync

@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label Jun 6, 2024
Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>
Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>
Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>
Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>
Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>
Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>
@JoeLoser
Copy link
Collaborator

JoeLoser commented Jun 7, 2024

!sync

@modularbot
Copy link
Collaborator

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

@modularbot modularbot added the merged-internally Indicates that this pull request has been merged internally label Jun 10, 2024
modularbot added a commit that referenced this pull request Jun 11, 2024
…le (#41582)

[External] [stdlib] Implement `NamedTemporaryFile` in `tempfile` module

Implementation of `NamedTemporaryFile` a wrapper around a `FileHandle`
that can be used as a context manager that is deleted after closing it
if needed.

ORIGINAL_AUTHOR=artemiogr97
<57588855+artemiogr97@users.noreply.github.com>
Closes #2762
MODULAR_ORIG_COMMIT_REV_ID: 84f0675e5d24afecb64a776cddc0a98823c2961a
@modularbot modularbot added the merged-externally Merged externally in public mojo repo label Jun 11, 2024
@modularbot
Copy link
Collaborator

Landed in fbc2595! Thank you for your contribution 🎉

@modularbot modularbot closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported-internally Signals that a given pull request has been imported internally. merged-externally Merged externally in public mojo repo merged-internally Indicates that this pull request has been merged internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants