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.

Sign up for GitHub

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

Jump to bottom

LocalModelStorage bypasses windows path name length restriction upon saving archive #11792

Merged
merged 6 commits into from
Dec 1, 2022
Merged

LocalModelStorage bypasses windows path name length restriction upon saving archive #11792

merged 6 commits into from
Dec 1, 2022

Conversation

m-vdb
Copy link
Collaborator

@m-vdb m-vdb commented Nov 23, 2022 •
edited
Loading

Proposed changes:

  • LocalModelStorage bypasses windows path name length restriction upon saving archive
  • Made sure the test was failing using TDD (see build logs)

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@m-vdb m-vdb force-pushed the CORE-50-fix-long-filename-issues-in-windows branch 2 times, most recently from 7c3dcc0 to c433ade Compare November 24, 2022 08:49
@m-vdb
Copy link
Collaborator Author

m-vdb commented Nov 24, 2022

@mvielkind would you care to share how you tested the long file path fix using \\\\?\\? I tried different things, and it seems that the built-in open() method doesn't even recognise the prefix when trying to open the file and write into it, and I get an OSError: [Errno 22] Invalid argument. You can see the error in the CI job with the logs there. I was able to reproduce the same error from a Windows VM, but couldn't find a way to fix it; and of course I couldn't find documentation of the issue out there....

@m-vdb m-vdb force-pushed the CORE-50-fix-long-filename-issues-in-windows branch from c433ade to e301e1d Compare November 25, 2022 08:25
@m-vdb
Copy link
Collaborator Author

m-vdb commented Nov 25, 2022

actually I hit another limit, described here. Each path component on windows cannot have a length > 255...

@m-vdb m-vdb force-pushed the CORE-50-fix-long-filename-issues-in-windows branch 4 times, most recently from 51489a0 to b9f7458 Compare November 28, 2022 09:41
@m-vdb m-vdb marked this pull request as ready for review November 28, 2022 10:39
@m-vdb m-vdb requested review from a team as code owners November 28, 2022 10:39
@m-vdb m-vdb requested review from znat, miraai and tmbo and removed request for a team, znat, miraai and tmbo November 28, 2022 10:39
@m-vdb m-vdb force-pushed the CORE-50-fix-long-filename-issues-in-windows branch from 8f24652 to b66e7f4 Compare November 28, 2022 11:10
@m-vdb m-vdb force-pushed the CORE-50-fix-long-filename-issues-in-windows branch 2 times, most recently from 9ec1912 to f188130 Compare November 28, 2022 14:23
@m-vdb m-vdb force-pushed the CORE-50-fix-long-filename-issues-in-windows branch from f188130 to 1029e39 Compare November 28, 2022 14:43
m-vdb
m-vdb commented Nov 28, 2022
View reviewed changes
rasa/engine/storage/local_model_storage.py
@@ -26,6 +27,32 @@
MODEL_ARCHIVE_METADATA_FILE = "metadata.json"


@contextmanager
def windows_safe_temporary_directory(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

from a code organisation standpoint, not sure if this should be in another utils file or stay here 🤔

Copy link
Member

Choose a reason for hiding this comment

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

since it is only used in here, i don't see the need to move it somewhere else

@m-vdb m-vdb requested a review from tmbo November 28, 2022 14:44
@github-actions
Copy link
Contributor

Status of the run: Failed

Commit: 7b53a0e, The full report is available as an artifact.

Datadog dashboard link

tmbo
tmbo approved these changes Nov 30, 2022
View reviewed changes
Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

lgtm - thanks a lot for digging into this one 🔥

rasa/engine/storage/local_model_storage.py
@@ -26,6 +27,32 @@
MODEL_ARCHIVE_METADATA_FILE = "metadata.json"


@contextmanager
def windows_safe_temporary_directory(
Copy link
Member

Choose a reason for hiding this comment

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

since it is only used in here, i don't see the need to move it somewhere else

@tmbo
Copy link
Member

tmbo commented Nov 30, 2022

before you merge, please add a changelog entry.

@tmbo
Copy link
Member

tmbo commented Nov 30, 2022

this should be released together with #11819 as that should hopefully be ready tomorrow as well

@m-vdb m-vdb enabled auto-merge December 1, 2022 08:54
@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

🚀 A preview of the docs have been deployed at the following URL: https://11792--rasahq-docs-rasa-v2.netlify.app/docs/rasa

@m-vdb m-vdb merged commit 956f537 into 3.3.x Dec 1, 2022
@m-vdb m-vdb deleted the CORE-50-fix-long-filename-issues-in-windows branch December 1, 2022 10:05
@m-vdb m-vdb mentioned this pull request Dec 1, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmbo tmbo tmbo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@m-vdb @tmbo