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

Fixed broken path joins and unclosed files #1709

Merged
merged 9 commits into from
Aug 16, 2022
Merged

Conversation

DPeled
Copy link
Contributor

@DPeled DPeled commented Jun 24, 2022

Description

After a bug occurred to me while running over Windows, I found out that there are a lot places in the code where there is a path join using string concatenation and not with os.path.join which creates some broken paths and results some errors while serving models. In my ways of changing it, I've noticed that there are some bugs of unclosed file descriptors that were just left open, so I clodes them too.

Fixes #1708, fixes #1686

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Feature/Issue validation/testing

Checklist:

  • Did you have fun?
  • Have you added tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?

changed the abs paths joins to use only os.path.join
@msaroufim msaroufim added windows p1 mid priority labels Jul 4, 2022
Copy link
Member

@msaroufim msaroufim left a comment

Choose a reason for hiding this comment

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

Very cool thank you for improving the Torchserve user experience on Windows. I still see a few stragglers left. Might as well fix those as well and we should be able to easily merge this

test/pytest/test_gRPC_inference_api.py Show resolved Hide resolved
test/pytest/test_gRPC_management_apis.py Show resolved Hide resolved
test/pytest/test_metrics_kf.py Outdated Show resolved Hide resolved
test/pytest/test_pytorch_profiler.py Outdated Show resolved Hide resolved
test/pytest/test_utils.py Show resolved Hide resolved
@msaroufim
Copy link
Member

Hi @DPeled are you still interested in finishing this PR? Just a few minor comments left and we can merge

@DPeled
Copy link
Contributor Author

DPeled commented Jul 13, 2022

@msaroufim I'll work on it soon, I didn't have much time at the last weeks

@DPeled
Copy link
Contributor Author

DPeled commented Jul 15, 2022

@msaroufim @lxning I've read your review and I replied as well.
Please take a look hopefully you'll understand my answers and why I answered it.

@DPeled DPeled requested review from msaroufim and lxning July 16, 2022 09:46
@msaroufim msaroufim requested review from mreso and agunapal July 22, 2022 00:31
Copy link
Collaborator

@mreso mreso left a comment

Choose a reason for hiding this comment

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

LGTM
@msaroufim Should we suggest usage of pathlib instead of os.path for future developments?

@msaroufim
Copy link
Member

Probably a good boot camp task template we can setup @mreso

Copy link
Collaborator

@agunapal agunapal 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

@DPeled
Copy link
Contributor Author

DPeled commented Aug 7, 2022

@msaroufim Hi is there something to do with the tasks that failed?
From the logs I can see that because some packages not installed or torchserver.exe was not recognized in windows.
It doesn't seems to fail as a result from my PR

@msaroufim
Copy link
Member

I kicked off CI again let's see what happens

@codecov
Copy link

codecov bot commented Aug 7, 2022

Codecov Report

Merging #1709 (49cc745) into master (aa2468d) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1709   +/-   ##
=======================================
  Coverage   45.28%   45.28%           
=======================================
  Files          64       64           
  Lines        2597     2597           
  Branches       60       60           
=======================================
  Hits         1176     1176           
  Misses       1421     1421           
Impacted Files Coverage Δ
ts/model_server.py 0.00% <ø> (ø)
ts/model_loader.py 81.17% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@msaroufim
Copy link
Member

@DPeled can you please fix the lint error with pre-commit instructions are the bottom of the failed action. Also the GPU check needs to pass

@msaroufim msaroufim merged commit 62b9f22 into pytorch:master Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1 mid priority windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unclosed file descriptors in tests Error in loading model in Windows os
6 participants