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

Document that s3 upload_fileobj closes the file obj #3150

Open
mdavis-xyz opened this issue Feb 17, 2022 · 7 comments · May be fixed by #3968
Open

Document that s3 upload_fileobj closes the file obj #3150

mdavis-xyz opened this issue Feb 17, 2022 · 7 comments · May be fixed by #3968
Labels
documentation This is a problem with documentation. feature-request This issue requests a feature. p2 This is a standard priority issue s3

Comments

@mdavis-xyz
Copy link
Contributor

When I look at the documentation for functions like upload_fileobj, it's not clear that boto3 will close the file for me.

I have a use case where I have a zip file in a BytesIO, I want to upload it and unzip it.
I assumed boto wouldn't close the file for me, but it does.
That's fine, since the code unzipping this won't close it, so I can just swap the order.

But the documentation should clarify what the behavior is, right there where it explains that the argument is a file-like object.

e.g.

Fileobj (a file-like object) -- A file-like object to upload. At a minimum, it must implement the read method, and must return bytes. This function will have .close() called on it.

@mdavis-xyz mdavis-xyz added feature-request This issue requests a feature. needs-triage This issue or PR still needs to be triaged. labels Feb 17, 2022
@tim-finnigan tim-finnigan self-assigned this Feb 17, 2022
@tim-finnigan tim-finnigan added s3 and removed needs-triage This issue or PR still needs to be triaged. labels Feb 17, 2022
@tim-finnigan
Copy link
Contributor

Hi @mdavis-xyz thanks for reaching out. This was reported awhile ago in another issue (#929) and an issue was created in the s3transfer repository to address it: boto/s3transfer#80. I’m going to close this as a duplicate and we can continue to track this in the s3transfer issue.

@tim-finnigan tim-finnigan added the duplicate This issue is a duplicate. label Feb 17, 2022
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@mdavis-xyz
Copy link
Contributor Author

@tim-finnigan I don't think this ticket should be closed.

The issue is that the documentation for boto3 does not mention an unintuitive behavior of boto3. That's still an issue. Those other Github issues for other repositories won't fix that doc issue for this repository.

As an end user it has never been clear to me what the difference is between botocore, s3transfer and boto3. (Perhaps that's the doc change that's required?) I don't think it's reasonable to assume that users will think to look deep into the docs of 2 dependencies of boto3 to find this behavior. But if they did, they still wouldn't find it, because that's not what those other issues are for.

That 80 issue in botocore has remained open for half a decade. I don't think that's going to change any time soon. It's not even clear what that issue is about. (is it about changing the docs to describe the file closing behavior, or changing the behavior to not close the file?)

If I submit a PR to just add one sentence to the boto3 docs, will you accept it?

@tim-finnigan tim-finnigan reopened this Feb 23, 2022
@tim-finnigan tim-finnigan added documentation This is a problem with documentation. and removed duplicate This issue is a duplicate. labels Feb 23, 2022
@tim-finnigan
Copy link
Contributor

Hi @mdavis-xyz thanks for following up. I brought this up for discussion with the team and they agreed that a short reference to this behavior could be added to the documentation if you want to create a PR. There is still some debate over whether the s3transfer issue should be considered a bug. But someone did share a workaround here in the comments.

@aBurmeseDev aBurmeseDev added response-requested Waiting on additional information or feedback. p3 This is a minor priority issue labels Nov 4, 2022
@github-actions
Copy link

github-actions bot commented Nov 4, 2022

Greetings! It looks like this issue hasn’t been active in longer than five days. We encourage you to check if this is still an issue in the latest release. In the absence of more information, we will be closing this issue soon. If you find that this is still a problem, please feel free to provide a comment or upvote with a reaction on the initial post to prevent automatic closure. If the issue is already closed, please feel free to open a new one.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Nov 4, 2022
@github-actions github-actions bot closed this as completed Nov 6, 2022
@matt-telstra
Copy link

Where should I put the comments?

Because Boto3 is programmatically generated from the service definition files, I find it incredibly hard to navigate the codebase and find out where anything is, or how it works.

@tim-finnigan tim-finnigan reopened this Nov 24, 2023
@tim-finnigan tim-finnigan removed their assignment Nov 24, 2023
@tim-finnigan tim-finnigan added p2 This is a standard priority issue and removed response-requested Waiting on additional information or feedback. closed-for-staleness p3 This is a minor priority issue labels Nov 24, 2023
@Foxicution
Copy link

Not sure how PR notifications work, just wanted to quickly mention I added a PR updating the docs (that hopefully closes this issue) @tim-finnigan #3968

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This is a problem with documentation. feature-request This issue requests a feature. p2 This is a standard priority issue s3
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants