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

Fixing onnx saving path bug #13959

Merged

Conversation

ahmedlone127
Copy link
Contributor

FIxes saving of Onnx models on windows

Description

Currently, Onnx models can't be saved on windows because of the temporary path being used. This PR fixes the temporary path by replacing the character which can raise an exception while saving

How Has This Been Tested?

Tested locally by saving models and reloading them on windows

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Code improvements with no or little impact
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING page.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@DevinTDHa
Copy link
Member

DevinTDHa commented Sep 11, 2023

@maziyarpanahi @ahmedlone127 I pushed a fix, which addresses the underlying issue.

The reason why it used to work for TF but not for onnx is the following:

In tf we always had a name for the graph already defined (saved_model.pb). We don't have that. Somewhere down the line we passed the whole path to onnxWrapper.saveToFile(onnxFile). The method saveToFile then receives this whole path and creates another tmp folder with it, which appends the whole path (pretty deep nesting).

  • In unix this is ok as we have paths that start with /.
  • In Windows however it starts with the drive. So we have something like C:/tmp_uuid/C:/tmp_uuid2 which causes this issue.

@DevinTDHa DevinTDHa requested review from maziyarpanahi and removed request for DevinTDHa September 11, 2023 12:19
@maziyarpanahi maziyarpanahi changed the base branch from master to release/511-release-candidate September 11, 2023 14:16
@maziyarpanahi maziyarpanahi merged commit 04f2f7e into release/511-release-candidate Sep 11, 2023
maziyarpanahi added a commit that referenced this pull request Sep 11, 2023
…date

* [SPARKNLP-906] Fix reading suffix (#13945)

* Sparknlp 888 Add ONNX support to MPNet embeddings (#13955)

* adding onxx support to mpnet

* remove name in test

* updating default name for mpnet models in scala and python

* updating default model name

* Adding ONNX Support to ALBERT Token and Sequence Classification and Question Answering annotators (#13956)

* SPARKNLP-891 Adding ONNX support for AlbertQuestionAnswering
SPARKNLP-892 Adding ONNX support for AlbertSequenceClassification
SPARKNLP-893 Adding ONNX support for AlbertTokenClassification

* SPARKNLP-891 Adding ONNX support for AlbertQuestionAnswering
SPARKNLP-892 Adding ONNX support for AlbertSequenceClassification
SPARKNLP-893 Adding ONNX support for AlbertTokenClassification

* SPARKNLP-884 Enabling getVectors method to get word vectors as spark dataframe (#13957)

* [SPARKNLP-890] ONNX E5 MPnet example (#13958)

* Bump version to 5.1.1

* [SPARKNLP-891] [SPARKNLP-892] [SPARKNLP-893] Adding docs for ONNX support in AlbertXXX

* Fix misspelling [skip test]

* Fixing onnx saving path bug (#13959)

* fixing onnx write issue on windows

* fixing indentation

* fixing formatting

* fixing formatting

* final formatting fix

* Fix onnx saving bug

---------

Co-authored-by: Devin Ha <t.ha@tu-berlin.de>
Co-authored-by: Maziyar Panahi <maziyar.panahi@iscpif.fr>

---------

Co-authored-by: Devin Ha <33089471+DevinTDHa@users.noreply.github.com>
Co-authored-by: ahmedlone127 <ahmedlone127@gmail.com>
Co-authored-by: Danilo Burbano <37355249+danilojsl@users.noreply.github.com>
Co-authored-by: Danilo Burbano <danilo@johnsnowlabs.com>
Co-authored-by: Devin Ha <t.ha@tu-berlin.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants