Use same defaults as `pandas.DataFrame.to_csv` in `kedro_datasets.pandas.csv_dataset.CSVDataset`.

[galenseilisnh]

Description

I ran a pipeline and saved the results only to find out that the index, which contained required information, was missing!

Context

Users are expecting the pandas.CSVDataset to be a shallow wrapper around pandas.DataFrame.to_csv. It essentially is, but along with that shallow wrapper expectation is the expectation that the defaults will be the same. I found it surprising that the defaults were not preserved. It isn't a huge deal, but it becomes a "gotcha" for anyone onboarding Kedro + pandas CSV.

Possible Alternatives

Naturally, users 'can' use the code in the current state. This isn't a huge problem. Just a footgun for new users.

[rashidakanchwala]

Thanks for flagging this — good point. Ideally, we should match Pandas, but changing this now would be a breaking change. I think the best path is to update the docstring and docs to make this clearer for users.

@merelcht what are your thoughts on this issue?

[galenseilisnh]

I think the best path is to update the docstring and docs to make this clearer for users.

That's a practical solution for the short term. In the long term I'd prefer this change be made in a major release.

Kedro users, including myself, will often accept breaking changes like this if they are communicated in advance and don't require substantial work to fix from the user end of things. The user code that would have to change here could be done for a large number of projects by a script, so from my point of view the changeover costs would be minor even for a large number of projects. That's my opinion as a user of Kedro, and a developer of some internal tools in Python, anyway. Hopefully it is useful to hear that.

[merelcht]

If the changes only need to happen in kedro-dataset, breaking it is no big deal. The next release is breaking anyway.

[merelcht]

@galenseilisnh , just to clarify is this specifically for the value "index": False ?

[galenseilisnh]

@galenseilisnh , just to clarify is this specifically for the value "index": False ?

@merelcht Good question. Yes, that is the parameter and argument implicit in the original post.

[astrojuanlu]

Those defaults have been like that since "forever" https://github.com/kedro-org/kedro/commit/b7b3301d38fa2f59b04d08dc279617eef42a8142, but the actual rationale is lost. We are okay changing them.

The task is to make this an empty dictionary:

https://github.com/kedro-org/kedro-plugins/blob/3ff2314d7903cf1ee50ce31ad99fcd2e21d9003c/kedro-datasets/kedro_datasets/pandas/csv_dataset.py#L74

@galenseilisnh would you like to send a PR?

[galenseilisnh]

@astrojuanlu Sure, I will find some time in the next couple of weeks. It looks an unsubstantial implementation effort. I just need to sit down and review the contributing standards and ensure I follow all the required steps.

[deepyaman]

Those defaults have been like that since "forever" kedro-org/kedro@b7b3301, but the actual rationale is lost. We are okay changing them.

Actually rationale isn't lost; the reason is because this allows you to more symmetrically save and load (i.e. the index doesn't end up as another non-index column when you reload).

You can save index and then specify index column to get the behavior you want. For now, I disagree with making this change, as it is quite significant given how long this behavior has been in place--and I think it's very justifiable.

[astrojuanlu]

When I said that it was lost, I mean that the discussion in the original PR https://github.com/McK-Private/private-kedro/pull/355 (sorry, private link) doesn't include any mention of this, and there's no comment in the codebase (which would hopefully help communicate intent).

Thanks for bringing it up regardless @deepyaman . Pasting @galenseilisnh original quote here:

Users are expecting the pandas.CSVDataset to be a shallow wrapper around pandas.DataFrame.to_csv. It essentially is, but along with that shallow wrapper expectation is the expectation that the defaults will be the same. I found it surprising that the defaults were not preserved. It isn't a huge deal, but it becomes a "gotcha" for anyone onboarding Kedro + pandas CSV.

Is there a chance that more users are being tripped up by this?

[datajoely]

I think the big issue is when you transcode between Spark, Pandas, Polars etc. I like this default behaviour, we can throw a warning if people really feel strongly.

[galenseilisnh]

Those defaults have been like that since "forever" kedro-org/kedro@b7b3301, but the actual rationale is lost. We are okay changing them.

Actually rationale isn't lost; the reason is because this allows you to more symmetrically save and load (i.e. the index doesn't end up as another non-index column when you reload).

You can save index and then specify index column to get the behavior you want. For now, I disagree with making this change, as it is quite significant given how long this behavior has been in place--and I think it's very justifiable.

That's an interesting point. I am going to think about that some more.

[galenseilisnh]

Reviewing the public API docs I see that correct and useful info is present:

https://docs.kedro.org/projects/kedro-datasets/en/kedro-datasets-7.0.0/api/kedro_datasets.pandas.CSVDataset.html

I have a hunch that I would have been more likely to spot this if the class-level docstring explained this deviation from Pandas' defaults. Based on this hunch, I suggest an explanation should go up here:

[galenseilisnh]

When I said that it was lost, I mean that the discussion in the original PR https://github.com/McK-Private/private-kedro/pull/355 (sorry, private link) doesn't include any mention of this, and there's no comment in the codebase (which would hopefully help communicate intent).

Thanks for bringing it up regardless @deepyaman . Pasting @galenseilisnh original quote here:

Users are expecting the pandas.CSVDataset to be a shallow wrapper around pandas.DataFrame.to_csv. It essentially is, but along with that shallow wrapper expectation is the expectation that the defaults will be the same. I found it surprising that the defaults were not preserved. It isn't a huge deal, but it becomes a "gotcha" for anyone onboarding Kedro + pandas CSV.

Is there a chance that more users are being tripped up by this?

I also think a code comment in kedro_datasets.pandas.CSVDataset would be helpful for some users.

[galenseilisnh]

After seeing some counterpoints I am convinced that we should keep the current state for the functional code. But I also think this is something that could cause future confusion (it confused me at first, FWIW), so further docs/comments would be my current suggestion.

[astrojuanlu]

Thanks @galenseilisnh for the extra pointers.

However, now it's me who needs to be convinced that "discard data" is a good default 😄

[astrojuanlu]

If the intent is to have save + load symmetry, why not leave index=True and make index_col=0 on read_csv?

In [3]: df
Out[3]: 
       value
index       
A         11
B         12
C         13

In [4]: df.to_csv("data.csv")

In [5]: pd.read_csv("data.csv")  # Ugh
Out[5]: 
  index  value
0     A     11
1     B     12
2     C     13

In [6]: pd.read_csv("data.csv", index_col=0)  # Recovers initial result
Out[6]: 
       value
index       
A         11
B         12
C         13

[galenseilisnh]

If the intent is to have save + load symmetry, why not leave index=True and make index_col=0 on read_csv?

In [3]: df
Out[3]: 
       value
index       
A         11
B         12
C         13

In [4]: df.to_csv("data.csv")

In [5]: pd.read_csv("data.csv")  # Ugh
Out[5]: 
  index  value
0     A     11
1     B     12
2     C     13

In [6]: pd.read_csv("data.csv", index_col=0)  # Recovers initial result
Out[6]: 
       value
index       
A         11
B         12
C         13

Good point. That also seems to obtain the IO symmetry.

[deepyaman]

If the intent is to have save + load symmetry, why not leave index=True and make index_col=0 on read_csv?

In [3]: df
Out[3]: 
       value
index       
A         11
B         12
C         13

In [4]: df.to_csv("data.csv")

In [5]: pd.read_csv("data.csv")  # Ugh
Out[5]: 
  index  value
0     A     11
1     B     12
2     C     13

In [6]: pd.read_csv("data.csv", index_col=0)  # Recovers initial result
Out[6]: 
       value
index       
A         11
B         12
C         13

~I think this would be fine.~

When this dataset was initially developed (and I was primarily a Kedro user), most users didn't use pandas indexes (I remember going through a phase trying to convince people that using indexes was a good thing to do, to no avail :joy:). As a result, it probably seemed most useful to not write a useless column nobody ever used. As a result, I think it's still worth being confident more people are actually using indexes before making this change (if at all).

As I was writing this response, I was like, "What about multi-indexes?" I think it's hard to get right.

[deepyaman]

That said, re I/O symmetry, pandas.ParquetDataset writes the index and would load it back as a column...

So the argument is a bit leaky, and there's definitely inconsistencies...

[galenseilisnh]

As a result, it probably seemed most useful to not write a useless column nobody ever used. As a result, I think it's still worth being confident more people are actually using indexes before making this change (if at all).

Good point.

One package I am using which uses pandas' indexing is sktime. I don't have any reliable quantification of use of this package, although it has over 8k stars.