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

Enable TensorFlowModelDataset to overwrite existing model, and add support for tf.device #1915

Merged
merged 13 commits into from
Oct 22, 2022

Conversation

williamcaicedo
Copy link
Contributor

@williamcaicedo williamcaicedo commented Oct 12, 2022

Description

Development notes

For models using the SavedModel format, a check was added to find out if the save path already exists. In case it does, path is deleted prior to saving the new model.

Removed the code that manually created all save path intermediate directories if they didn't exist, as _fs_args.setdefault("auto_mkdir", True) is already being set.

Model uses gpu by default unless tf_device: cpu load arg is supplied.

Checklist

  • [*] Read the contributing guidelines
  • [*] 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 RELEASE.md file
  • [*] Added tests to cover my changes

@merelcht merelcht changed the title Fix 696 tfmodeldataset Fix TensorFlowModelDataset can't override existing local model Oct 12, 2022
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.

Took a quick pass; shouldn't have started at 1:30a, so going to leave it at that for now. 😅 Happy to take a look at any responses later.

RELEASE.md Outdated Show resolved Hide resolved
dinotuku and others added 8 commits October 13, 2022 12:58
* Change a file extention to match the previous article

Signed-off-by: dinotuku <kuan.tung@epfl.ch>

* Add a missing import

Signed-off-by: dinotuku <kuan.tung@epfl.ch>

* Change both preprocessed datasets to parquet files

Signed-off-by: dinotuku <kuan.tung@epfl.ch>

* Change data type to ParquetDataSet for parquet files

Signed-off-by: dinotuku <kuan.tung@epfl.ch>

* Add a note for installing seaborn if it is not installed

Signed-off-by: dinotuku <kuan.tung@epfl.ch>

Signed-off-by: dinotuku <kuan.tung@epfl.ch>
Signed-off-by: William Caicedo <williamc@movio.co>
Signed-off-by: William Caicedo <williamc@movio.co>
Signed-off-by: William Caicedo <williamc@movio.co>
Signed-off-by: William Caicedo <williamc@movio.co>
Signed-off-by: William Caicedo <williamc@movio.co>
Signed-off-by: William Caicedo <williamc@movio.co>
Signed-off-by: William Caicedo <williamc@movio.co>
Co-authored-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: William Caicedo <williamc@movio.co>
@williamcaicedo williamcaicedo requested review from deepyaman and removed request for idanov and yetudada October 17, 2022 21:10
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.

I haven't tested this myself yet, but LGTM from a code review perspective!

Edit: @williamcaicedo I added a note to PR title that added support for tf.device; can you actually include that in the release notes, too?

@deepyaman deepyaman changed the title Fix TensorFlowModelDataset can't override existing local model Enable TensorFlowModelDataset to override existing model, and add support for tf.device Oct 19, 2022
@deepyaman deepyaman changed the title Enable TensorFlowModelDataset to override existing model, and add support for tf.device Enable TensorFlowModelDataset to overwrite existing model, and add support for tf.device Oct 19, 2022
@noklam noklam added the Community Issue/PR opened by the open-source community label Oct 20, 2022
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.

Thank you, this PR is very much appreciated⭐️. Haven't tried to run myself, but looking at the test it seems working fine, plus it's not working previously so I am not concern about this.

@williamcaicedo
Copy link
Contributor Author

I haven't tested this myself yet, but LGTM from a code review perspective!

Edit: @williamcaicedo I added a note to PR title that added support for tf.device; can you actually include that in the release notes, too?

@deepyaman done 👍

@deepyaman
Copy link
Member

I haven't tested this myself yet, but LGTM from a code review perspective!
Edit: @williamcaicedo I added a note to PR title that added support for tf.device; can you actually include that in the release notes, too?

@deepyaman done 👍

Amazing, thanks!

@deepyaman deepyaman merged commit cbe0522 into kedro-org:main Oct 22, 2022
nickolasrm pushed a commit to ProjetaAi/kedro that referenced this pull request Oct 26, 2022
…support for `tf.device` (kedro-org#1915)

* Fix issue with save operation. Add gpu option

Signed-off-by: William Caicedo <williamc@movio.co>

* Add tests

Signed-off-by: William Caicedo <williamc@movio.co>

* Update RELEASE.md

Signed-off-by: William Caicedo <williamc@movio.co>

* Update test description

Signed-off-by: William Caicedo <williamc@movio.co>

* Remove double slash and overwrite flag in fsspec.put method invocation

Signed-off-by: William Caicedo <williamc@movio.co>

* Allow to explicitly set device name

Signed-off-by: William Caicedo <williamc@movio.co>

* Update RELEASE.md

Co-authored-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: William Caicedo <williamc@movio.co>

* Update docs

Signed-off-by: William Caicedo <williamc@movio.co>
Co-authored-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: nickolasrm <nickolasrochamachado@gmail.com>
mle-els pushed a commit to mle-els/kedro that referenced this pull request Nov 7, 2022
…support for `tf.device` (kedro-org#1915)

* Fix issue with save operation. Add gpu option

Signed-off-by: William Caicedo <williamc@movio.co>

* Add tests

Signed-off-by: William Caicedo <williamc@movio.co>

* Update RELEASE.md

Signed-off-by: William Caicedo <williamc@movio.co>

* Update test description

Signed-off-by: William Caicedo <williamc@movio.co>

* Remove double slash and overwrite flag in fsspec.put method invocation

Signed-off-by: William Caicedo <williamc@movio.co>

* Allow to explicitly set device name

Signed-off-by: William Caicedo <williamc@movio.co>

* Update RELEASE.md

Co-authored-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: William Caicedo <williamc@movio.co>

* Update docs

Signed-off-by: William Caicedo <williamc@movio.co>
Co-authored-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Minh Le <m.le@elsevier.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Issue/PR opened by the open-source community
Projects
None yet
5 participants