REGR: be able to read Stata files without reading them fully into memory
State: closed

Fixes #48700
Refs pandas-dev/pandas#9245
Refs pandas-dev/pandas#37639
Regressed in 6d1541e1782a7b94797d5432922e64a97934cfa4


- [x] closes #48700 (Replace xxxx with the Github issue number)
- [x] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
  - [x] The existing tests for e.g. roundtripping zstandard-compressed Stata files test this code path.
  - [x] Added a test that checks e.g. a fp or BytesIO passed in is the same object the reader reads. 
- [x] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [x] Added [type annotations](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#type-hints) to new arguments/methods/functions.
  - Nothing new to add. 
- [x] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.


Labels:
name: Performance
description: Memory or execution speed performance
name: IO Stata
description: read_stata, to_stata
name: Stale

Comments:
There seem to be some failing tests on windows:

> FAILED pandas/tests/io/test_stata.py::TestStata::test_utf8_writer[118] - Perm...
> FAILED pandas/tests/io/test_stata.py::TestStata::test_utf8_writer[119] - Perm...
> FAILED pandas/tests/io/test_stata.py::TestStata::test_utf8_writer[None] - Per...
> FAILED pandas/tests/io/test_stata.py::test_non_categorical_value_labels - Per...
> FAILED pandas/tests/io/test_stata.py::test_non_categorical_value_label_name_conversion
> FAILED pandas/tests/io/test_stata.py::test_non_categorical_value_label_convert_categoricals_error
> There seem to be some failing tests on windows:

Right, e.g.

```
       try:
>           self._accessor.unlink(self)
E           PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\32b05830-5662-48d4-9c1a-e0a1002e813c'
```

looks like not all handles are being correctly, which is apparently a test suite bug, fixed in https://github.com/pandas-dev/pandas/pull/48922/commits/99d3540e04133d65ae564caecc9318015ae8f40e :)
LGTM cc @bashtage 
As for a way forward, _maybe_ could consider adding a keyword argument `buffer: {True, False, None (or "auto")}` with a default of lib.NoDefault.  

If `True`, then continue with the current behavior.  If `False` use the new behavior.  If `None` or `"auto"`, let StattaReader decide. If `lib.NoDefault` then treat as `True` and issue a warning that there will be a deprecation warning in some future version.  If *False* but the file requires buffering (e.g., a gzip), then either raise some sort of IOError or warn that buffering can't be used with files that do not support `seek`. `None` or `"auto"` will be the medium run-default. And possibly this keyword could be dropped in some very distant release after a second deprecation cycle. 

If using this strategy, then the current test suite would still pass unmodified.

I'm always -1 for adding to the API though. 
Another way forward would be to implement this **only** if the iterator is being used (_and_ it is possible).  The iterator is already self-closing when read to exhaustion, and this is the case where it is most important.  In other uses of `StataReader`, no change would occur. 

One could start a path towards requiring the user to self-close in general, but that needs Deprecation.  This would still improve memory use since it would not read the entire file effectively twice (once to the buffer, and then again when reading the values from the buffer). 
I'm not surprised that some tests fail - but they mostly cover CI-behavior, a user is very unlikely to do something like that:

`StataReader` is directly instantiated and `read` is not called (I think using `with` is definitely the solution here): 

> FAILED pandas/tests/io/test_stata.py::TestStata::test_utf8_writer[118] - Perm...
> FAILED pandas/tests/io/test_stata.py::TestStata::test_utf8_writer[119] - Perm...
> FAILED pandas/tests/io/test_stata.py::TestStata::test_utf8_writer[None] - Per...
> FAILED pandas/tests/io/test_stata.py::test_non_categorical_value_labels - Per...
> FAILED pandas/tests/io/test_stata.py::test_non_categorical_value_label_name_conversion

This one should be fixed: raises `ValueError` during `read_stata` but does not close the file handle. Probably needs some try/except.

> FAILED pandas/tests/io/test_stata.py::test_non_categorical_value_label_convert_categoricals_error
While the rebase obscures the changes to the test file, the code still needs to be modified only to apply when using an iterator. 
@bashtage

"Obscures the changes to the test file"? There are no more changes to existing tests in this PR, since #48931 (in `main`, on track for 1.6) fixed tests to use `with` wherever any objects adhering to the context manager protocol are used, including `StataReader`. This brings the tests for `StataReader` in line with other prior art of contextful reader objects in Pandas such as `ExcelFile`, `HDFStore`, `TextFileReader` and the SAS readers.

While rebasing, I did add a documentation note for the users of the underlying `StataReader` object that they need to close it, either with `.close()` or by using `with`, like the examples in the documentation and tests do.
> While rebasing, I did add a documentation note for the users of the underlying StataReader object that they need to close it, either with .close() or by using with, like the examples in the documentation and tests do.

I feel strongly that this is not enough to justify introducing a change that could break code. In the case of an iterator, it is necessary to introduce a potentially breaking change, although in the current implementation, this would only cause an issue in a relatively rare case IMO (iterator, don't read to exhaustion, and don't close).  In the case without an iterator, there is no need for this change at this point, so it is a case where the potential costs outweigh the potential benefits. 
> I feel strongly that this is not enough to justify introducing a change that could break code.

As we discussed before, the only code that could break is code that is using the mostly undocumented `StataReader` class directly without adhering to its context manager protocol (which is shown in all documentation related to `read_stata`, and now in all test code using `StataReader`).

The breakage that could occur for not using the CM protocol is, in my opinion, minor: a file handle left open, which will either
(a) just cause a `ResourceWarning` when Python garbage-collects the file handle before it is closed
(b) cause an exception on Windows if the user attempts to delete the file while it's still open.

If there are other plausible scenarios I'm missing here, let me know.

> In the case without an iterator, there is no need for this change at this point, so it is a case where the potential costs outweigh the potential benefits.

Sorry, but I think this is untrue. The status quo on `main`, for reading, say, the aforementioned 34 gigabyte stata file is that Pandas buffers it into memory (allocating 34 gigabytes), then starts parsing it into Pandas data structures, requiring possibly another 34 gigabytes (assuming no overhead and so on), before it finally releases the buffer, so you'd be looking at a peak allocation of double the file size, instead of "just" what the final dataframe requires.
> Sorry, but I think this is untrue. The status quo on main, for reading, say, the aforementioned 34 gigabyte stata file is that Pandas buffers it into memory (allocating 34 gigabytes), then starts parsing it into Pandas data structures, requiring possibly another 34 gigabytes (assuming no overhead and so on), before it finally releases the buffer, so you'd be looking at a peak allocation of double the file size, instead of "just" what the final dataframe requires.

To me this is pretty minor.  IIRC StataReader needs at least 2x memory even without buffering due to the way it inverts the raw data on disk to an appropriate DataFrame with correct dtypes, so with full buffering it would be around 3x.  3x -> 2x is small, even if it might just let someone knife edge in.  

In the case of an iterator, the memory savings can be very large though. Hence the support for this case. 

> The breakage that could occur for not using the CM protocol is, in my opinion, minor: a file handle left open, which will either
(a) just cause a ResourceWarning when Python garbage-collects the file handle before it is closed
(b) cause an exception on Windows if the user attempts to delete the file while it's still open.

IMO knowlingly making changes so that previously working code will lead to an exception without warning is not good practice. 


> To me this is pretty minor. [...] 3x -> 2x is small, even if it might just let someone knife edge in.

3x -> 2x doesn't sound small to me when we could be talking about gigabytes and laptop or desktop class machines with finite memory.

> IMO knowlingly making changes so that previously working code will lead to an exception without warning is not good practice.

IMO making changes that break code that have knowingly been misusing the library is not a big issue. And again, the only case where I can think an _exception_ will occur is attempting to move/rename/delete the file on Windows that is open in a misused `StataReader`. 

Also, looking at the <del>whatsnew/1.6.0.rst</del>whatsnew/2.0.0.rst file, there are already several changes that could break user code that has relied on certain behavior. I can't understand why this PR is such an exception to this, with the doc admonition and everything in place.
I had a different idea trying to find a way forward for this:

1. Defer the decision on how to open the file from the `__init__`
2. If the user uses the context manager, open the file using the changes in this PR.
3. If the user accesses contents without the context manager (`read`, `__next__`, `data_label`, `value_labels`,`variable_labels`) then show a UserWarning (or other appropriate class) telling them that they need to use the CM, that failing to use the CM requires buffering the entire file into memory.  It should also tell them that in the future they must use the CM since it is the only way to ensure that the file is closed after reading data. If they use this path, the file should be read into a buffer and immediately closed, as it currently is. This probably requires promoting some attributes to `property` so that a simple check can be performed.
4. In `read_stata`, the non-iterator method needs to use the CM.
5. Remove all of the uses of `close()` throughout `StataReader` since the file will either by (a) used with the CM or (b) fully buffered and closed.



@bashtage It looks like Pandas 1.6 is Pandas 2.0 – with a major version bump like that, people are even more expected to read the readme for possibly breaking changes. (And, again, I don't believe people use the undocumented `StataReader` class by hand that much.)

As such I'd say only point 5 from your proposal is really required, i.e. don't have StataReader itself decide when to close the handle (that it may not have opened by itself), but leave that to the users by way of CM/`.close()`, just like `ExcelFile`, `HDFStore`, `TextFileReader` and the SAS readers do.
Just because it is 2.0 doesn't mean we should go out of our way to increase breaks.  There is no good reason to not both provide an improvement for those using things correctly while not breaking and warning those who are not.  The entire patch would be ~15 LOC in StataReader.  
@bashtage And I don't believe we should go out of our way to complicate the library to cater for users who have been misusing it for 7 years (ever since the CM protocol was introduced into StataReader).

Anyway, as you feel so strongly about that backward compatibility (and apparently have a vision of how to implement it in ~15 LOC), could we maybe just get this finally accepted and merged, and you can follow up with a PR that fixes those things? 😄 
If you won't fix it here, I'll just write an alternative PR that does this, provides the other cleanups, and provides warnings to users who may be accessing things in an unsupported way.  I don't really understand the motivation to possibly break user code. 
> If you won't fix it here, I'll just write an alternative PR that does this [...]

That... sounds a bit unfriendly and unwelcoming, doesn't it? 😞 
I suggested we could get this merged and you could follow up with the additional changes you deem necessary.

I think deferring the opening handles to another function (an `_ensure_open` that needs to be called from every function that could access the handles?) and changing tests, etc. to accommodate for that would make this PR a bit longer and more complex than it needs to be to just do this one thing.

> I don't really understand the motivation to possibly break user code.

Well, to paraphrase you, you could call it an "coding-efficient choice"...

Anyway, as I've said multiple times, I believe people who have been using `StataReader` directly without a CM have been using it wrong ever since the CM protocol was introduced to it.

@akx havent been following that closely but the point is in pandas we really try to go out of our way to avoid breaking changes 

sometimes these increase code complexity - pandas is heavily used and changing things requires notice not just notice but deprecation periods if at all possible

@bashtage makes some good recommendations here
To be clear, I don't want to write a PR; I would like you to bring this one up to standards.  It is not about being unfriendly.  I am simply putting in my input into how this can be moved forward while balancing code complexity and technical debt, performance, and user friendliness.   A bit of noise for users who are incorrectly using the code, while protecting them, seems like a small lift, especially since anyone who is using it in the blessed way.  As to why I really think this is needed, look no further than the example in the `read_stata` docs:

```python
itr = pd.read_stata('filename.dta', chunksize=10000)  
for chunk in itr:
   # Operate on a single chunk, e.g., chunk. Mean()
   pass  
```

which does not use the CM.  I do not think that the officially supported way of using `StataReader` is discoverable by end users.  The docstring for `StataReader` also doesn't mention context manager (the work context is in stata.py twice, once in `__enter__` and once in `__exit__`).

Having looked through things I'd like to add a # 6 to the above list:

6. Correct the example in `read_stata` to use the context manager, and clarify in both the read_stata docs and the `StataReader` docstring that the context manager is the only way that it should be used. 


@akx pretty much all of the maintainers are going to defer to bashtage on this
> @akx pretty much all of the maintainers are going to defer to bashtage on this

Yeah no worries @jbrockmendel, we're continuing in #49228 (the v2 of this) :)
This pull request is stale because it has been open for thirty days with no activity. Please [update](https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#updating-your-pull-request) and respond to this comment if you're still interested in working on this.
Closing in favor of https://github.com/pandas-dev/pandas/pull/49228