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

feat: Add CSV export capability to DataFrameClient #45

Merged
merged 20 commits into from
Apr 6, 2023

Conversation

kjohn1922
Copy link
Contributor

What does this Pull Request accomplish?

Add export_table_data to the DataFrameClient, which calls the export route on the DataFrame Service API.

Wrap the response.iter_content generator in a file-like object that can either be written to a file or loaded into a pandas dataframe, without loading the data into memory all at once beforehand.

Added an example of writing the export data to a file or pandas.

Why should this Pull Request be merged?

Parity of the python API with new features added to the DataFrame Service API.

What testing has been done?

Manual testing using Jupyter notebooks. Added unit tests for the generator wrapper class. Added an integration test for the export data method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A couple of comments on this:

  • It's returning the data as binary data. The binary can be written to a CSV file or to pandas directly, but if the user wants to get the data as a string in code, they will need to decode it themselves. We did this in anticipation of adding binary export formats in the future (TDMS).
  • I didn't implement readline or readlines, since they are not required for writing to a file or to pandas. I can add them if we want this to be a fully-functional file-like object, but they were getting a bit messy with iterating the binary and normalizing line endings to what python expects.

@kjohn1922
Copy link
Contributor Author

I'm working through some type errors.

pyproject.toml Outdated
@@ -29,7 +29,7 @@ classifiers = [
]

[tool.poetry.dependencies]
python = "^3.8"
python = ">=3.8,<3.12"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was required to satisfy the mypy type checker for having pandas in the export-data example. mypy needs pandas-stubs to do type checking for pandas, and pandas-stubs requires python to be between these two versions.

If we're not happy with this, I can remove pandas-stubs and revert this line, because it just gives a warning from mypy that it's skipping type checking for pandas without it.

docs/api_reference/dataframe.rst Show resolved Hide resolved
docs/getting_started.rst Show resolved Hide resolved
examples/dataframe/export_data.py Outdated Show resolved Hide resolved
nisystemlink/clients/core/helpers/iterator_file_like.py Outdated Show resolved Hide resolved
nisystemlink/clients/core/helpers/iterator_file_like.py Outdated Show resolved Hide resolved
nisystemlink/clients/dataframe/_data_frame_client.py Outdated Show resolved Hide resolved
nisystemlink/clients/dataframe/_data_frame_client.py Outdated Show resolved Hide resolved
nisystemlink/clients/core/helpers/__init__.py Outdated Show resolved Hide resolved
tests/integration/dataframe/test_dataframe.py Outdated Show resolved Hide resolved
@mure
Copy link
Contributor

mure commented Mar 28, 2023

From https://dev.azure.com/ni/DevCentral/_workitems/edit/2347097,

If the server terminates the connection for any reason, the client will get an unhelpful requests.exceptions.ChunkedEncodingError error. It would be nice to catch this and have a more user friendly message.

pyproject.toml Outdated Show resolved Hide resolved
@kjohn1922
Copy link
Contributor Author

From https://dev.azure.com/ni/DevCentral/_workitems/edit/2347097,

If the server terminates the connection for any reason, the client will get an unhelpful requests.exceptions.ChunkedEncodingError error. It would be nice to catch this and have a more user friendly message.

We have a couple of ideas for how to solve this, a couple of which would be changes on the DFS side. I added some details to the bug, and we can decide on the approach when we get to it.

@kjohn1922 kjohn1922 requested a review from mure April 6, 2023 15:49
mypy.ini Outdated Show resolved Hide resolved
@kjohn1922 kjohn1922 merged commit 5cc2d01 into master Apr 6, 2023
@kjohn1922 kjohn1922 deleted the users/kelseyj/csv-export branch April 6, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants