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

fix(datasets): Refactor TensorFlowModelDataset to DataSet #186

Merged

Conversation

BrianCechmanek
Copy link
Contributor

@BrianCechmanek BrianCechmanek commented Apr 21, 2023

Matching consistency of all other kedro-datasets, DataSet should be camelcase.

It will be reverted in 0.19.0 per #2129.

Description

See previous discussion #2494.

All other supported DataSets have are camelcased (upper S).

Development notes

This refactors all instances of TensorFlowModelDataset to TensorFlowModelDataSet.
There are also some instances of TensorflowModelDataset, looking into if those should be changed as well (tbd)

I couldn't run the test suite to verify - I will investigate that later.

Checklist

  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the relevant RELEASE.md file
  • Added tests to cover my changes

Copy link
Member

@deepyaman deepyaman left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Looks good to me, maybe add this in the release note? I consider this as a bug fix.

@BrianCechmanek BrianCechmanek force-pushed the fix/dataset-naming-tensorflowmodeldataset branch from 0018cc4 to f5cbccd Compare April 25, 2023 13:52
BrianCechmanek and others added 4 commits April 25, 2023 15:00
matching consistency of all other kedro-datasets, DataSet should be camelcase. will be reverted in 0.19.0

Signed-off-by: BrianCechmanek <[email protected]>
Currently opening gitpod will installed a Python 3.11 which breaks everything because we don't support it set. This PR introduce a simple .gitpod.yml to get it started.

Signed-off-by: BrianCechmanek <[email protected]>
* Update APIDataSet

Signed-off-by: Nok Chan <[email protected]>

* Sync ParquetDataSet

Signed-off-by: Nok Chan <[email protected]>

* Sync Test

Signed-off-by: Nok Chan <[email protected]>

* Linting

Signed-off-by: Nok Chan <[email protected]>

* Revert Unnecessary ParquetDataSet Changes

Signed-off-by: Nok Chan <[email protected]>

* Sync release notes

Signed-off-by: Nok Chan <[email protected]>

---------

Signed-off-by: Nok Chan <[email protected]>
Signed-off-by: BrianCechmanek <[email protected]>
…edro-org#182)

* bump tables version and remove step in workflow

Signed-off-by: Ankita Katiyar <[email protected]>

* revert version for linux

Signed-off-by: Ankita Katiyar <[email protected]>

* change version to 3.7

Signed-off-by: Ankita Katiyar <[email protected]>

* remove extra line

Signed-off-by: Ankita Katiyar <[email protected]>

---------

Signed-off-by: Ankita Katiyar <[email protected]>
Signed-off-by: BrianCechmanek <[email protected]>
@BrianCechmanek BrianCechmanek force-pushed the fix/dataset-naming-tensorflowmodeldataset branch from f5cbccd to 41aee34 Compare April 25, 2023 14:00
@BrianCechmanek
Copy link
Contributor Author

Thanks both!

@noklam, not sure if you meant for me specifically to add a comment to the RELEASE.md.. Wasn't quite sure format, so I just guessed. Please feel free to fix/reject.

Note: I couldn't get any of the commands in the makefile working, specifically to build and/or test this PR locally. I'll try and look into this next.

@BrianCechmanek BrianCechmanek marked this pull request as ready for review April 25, 2023 14:36
@noklam noklam changed the title Refactor TensorFlowModelDataset to DataSet dataset: Refactor TensorFlowModelDataset to DataSet Apr 28, 2023
@noklam noklam changed the title dataset: Refactor TensorFlowModelDataset to DataSet fix(dataset): Refactor TensorFlowModelDataset to DataSet Apr 28, 2023
@noklam noklam changed the title fix(dataset): Refactor TensorFlowModelDataset to DataSet fix(datasets): Refactor TensorFlowModelDataset to DataSet Apr 28, 2023
@noklam
Copy link
Contributor

noklam commented Apr 28, 2023

@BrianCechmanek Yes, that's great. I have made a few more changes in the doc and I am merging it now! Thanks

@noklam noklam merged commit 435689f into kedro-org:main Apr 28, 2023
@BrianCechmanek BrianCechmanek deleted the fix/dataset-naming-tensorflowmodeldataset branch April 28, 2023 14:21
dannyrfar pushed a commit to dannyrfar/kedro-plugins that referenced this pull request May 3, 2023
)

* refactor TensorFlowModelDataset to Set

matching consistency of all other kedro-datasets, DataSet should be camelcase. will be reverted in 0.19.0

Signed-off-by: BrianCechmanek <[email protected]>

* Introdcuing .gitpod.yml to kedro-plugins (kedro-org#185)

Currently opening gitpod will installed a Python 3.11 which breaks everything because we don't support it set. This PR introduce a simple .gitpod.yml to get it started.

Signed-off-by: BrianCechmanek <[email protected]>

* sync APIDataSet  from kedro's `develop` (kedro-org#184)

* Update APIDataSet

Signed-off-by: Nok Chan <[email protected]>

* Sync ParquetDataSet

Signed-off-by: Nok Chan <[email protected]>

* Sync Test

Signed-off-by: Nok Chan <[email protected]>

* Linting

Signed-off-by: Nok Chan <[email protected]>

* Revert Unnecessary ParquetDataSet Changes

Signed-off-by: Nok Chan <[email protected]>

* Sync release notes

Signed-off-by: Nok Chan <[email protected]>

---------

Signed-off-by: Nok Chan <[email protected]>
Signed-off-by: BrianCechmanek <[email protected]>

* [kedro-datasets] Bump version of `tables` in `test_requirements.txt`  (kedro-org#182)

* bump tables version and remove step in workflow

Signed-off-by: Ankita Katiyar <[email protected]>

* revert version for linux

Signed-off-by: Ankita Katiyar <[email protected]>

* change version to 3.7

Signed-off-by: Ankita Katiyar <[email protected]>

* remove extra line

Signed-off-by: Ankita Katiyar <[email protected]>

---------

Signed-off-by: Ankita Katiyar <[email protected]>
Signed-off-by: BrianCechmanek <[email protected]>

* refactor tensorflowModelDataset casing in datasets setup.py

Signed-off-by: BrianCechmanek <[email protected]>

* add tensorflowmodeldataset bugfix to release.md

Signed-off-by: BrianCechmanek <[email protected]>

* Update all the doc reference with TensorFlowModelDataSet

Signed-off-by: Nok <[email protected]>

---------

Signed-off-by: BrianCechmanek <[email protected]>
Signed-off-by: Nok Chan <[email protected]>
Signed-off-by: Ankita Katiyar <[email protected]>
Signed-off-by: Nok <[email protected]>
Co-authored-by: Nok Lam Chan <[email protected]>
Co-authored-by: Ankita Katiyar <[email protected]>
Co-authored-by: Nok <[email protected]>
Signed-off-by: Danny Farah <[email protected]>
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.

4 participants