serve icon indicating copy to clipboard operation
serve copied to clipboard

Fixed broken path joins and unclosed files

Open DPeled opened this issue 3 years ago • 8 comments

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.

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

Feature/Issue validation/testing

Checklist:

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

DPeled avatar Jun 24 '22 14:06 DPeled

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

msaroufim avatar Jul 13 '22 21:07 msaroufim

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

DPeled avatar Jul 13 '22 21:07 DPeled

@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 avatar Jul 15 '22 15:07 DPeled

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

msaroufim avatar Jul 22 '22 04:07 msaroufim

@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

DPeled avatar Aug 07 '22 16:08 DPeled

I kicked off CI again let's see what happens

msaroufim avatar Aug 07 '22 19:08 msaroufim

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%> (ø)

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

codecov[bot] avatar Aug 07 '22 19:08 codecov[bot]

@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 avatar Aug 09 '22 16:08 msaroufim