Remove useless global variable in custom_utils #454
Conversation
Removing this variable allows to use dfferent instances of the same class (CustomImagePrediction) at the same time without using the same model_json.
Codecov Report
@@ Coverage Diff @@
## master #454 +/- ##
==========================================
+ Coverage 48.83% 49.95% +1.11%
==========================================
Files 63 63
Lines 5856 5855 -1
==========================================
+ Hits 2860 2925 +65
+ Misses 2996 2930 -66 |
Update requirements.txt
|
Build errors all come from a matplotlib backend issue: |
|
|
||
| if CLASS_INDEX is None: | ||
| CLASS_INDEX = json.load(open(model_json)) | ||
| CLASS_INDEX = json.load(open(model_json)) |
There was a problem hiding this comment.
I agree current implementation is not the best. However, this alternative isn't that better: each invocation on it will load exactly the same json.
If will let this change, should at least add a with block to make sure the file is closed properly:
with open(model_json) as f:
CLASS_INDEX = json.load(f)Agree on removing as much global variables as possible.
I don't fully understand why was on a global variable while the filename is a parameter... doesn't make sense for me, maybe I'm not seeing something.
There was a problem hiding this comment.
I agree current implementation is not the best. However, this alternative isn't that better: each invocation on it will load exactly the same json.
Yeah but that's my point actually. With several instances of CustomImagePrediction running, each invocation will not load exactly the same json and the global variable CLASS_INDEX prevents us from running these instances at the same time.
I don't fully understand why was on a global variable while the filename is a parameter... doesn't make sense for me, maybe I'm not seeing something.
This implementation is useful in other parts of codes in other "custom_utils.py" where some functions don't give a model_json in their parameters (but I checked, all functions that call this "custom_utils.py" give it).
PS : I may have misunderstood your concern, english is not my first language...
There was a problem hiding this comment.
My point is tat previous version loaded the file only once. We should go to an implementation with only one file-reading but avoiding the point you mention. A bigger refactor is needed, as I see.
English is not my first language too
thanks for your PR, we need to wait for @OlafenwaMoses now
|
with respect to the change in requirements.txt, we should fix versions for every library. must consider this for future releases @OlafenwaMoses |
Removing this variable allows to use dfferent instances of the same class (CustomImagePrediction) at the same time without using the same model_json.