Skip to content
This repository was archived by the owner on Aug 7, 2025. It is now read-only.

Fixed broken path joins and unclosed files #1709

Merged
merged 9 commits into from
Aug 16, 2022
Merged

Fixed broken path joins and unclosed files #1709

merged 9 commits into from
Aug 16, 2022

Conversation

@DPeled
Copy link
Contributor

@DPeled DPeled commented Jun 24, 2022 •
edited
Loading

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?

DPeled added 2 commits June 24, 2022 02:33
@DPeled
Refactor
77b01a7 
changed the abs paths joins to use only os.path.join
@DPeled
closing file descriptors that were never closed
a12db80
@msaroufim msaroufim requested review from maaquib and msaroufim June 24, 2022 16:24
@msaroufim msaroufim added windows p1 mid priority labels Jul 4, 2022
msaroufim
msaroufim suggested changes Jul 5, 2022
View reviewed changes
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

lxning
lxning reviewed Jul 8, 2022
View reviewed changes
@msaroufim msaroufim mentioned this pull request Jul 11, 2022
Closed
@msaroufim
Copy link
Member

msaroufim commented Jul 13, 2022

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 lxning and msaroufim July 16, 2022 09:46
@msaroufim msaroufim requested review from agunapal and mreso July 22, 2022 00:31
mreso
mreso reviewed Jul 22, 2022
View reviewed changes
mreso
mreso approved these changes Jul 22, 2022
View reviewed changes
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

msaroufim commented Jul 22, 2022

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

agunapal
agunapal approved these changes Jul 22, 2022
View reviewed changes
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

msaroufim commented Aug 7, 2022

I kicked off CI again let's see what happens

@codecov
Copy link

codecov bot commented Aug 7, 2022 •
edited
Loading

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

msaroufim commented Aug 9, 2022

@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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lxning lxning Awaiting requested review from lxning

4 more reviewers

@maaquib maaquib maaquib approved these changes

@msaroufim msaroufim msaroufim approved these changes

@mreso mreso mreso approved these changes

@agunapal agunapal agunapal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@DPeled DPeled

Labels

p1 mid priority windows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

6 participants

@DPeled @msaroufim @maaquib @mreso @agunapal @lxning