victoresque / pytorch-template

PyTorch deep learning projects made easy.
MIT License
4.7k stars 1.08k forks source link

Replacing usage of 'eval' in repository #23

Closed borgesa closed 5 years ago

borgesa commented 5 years ago

Hi,

While I was trying to figure out how to both enable users to use both "PyTorch" loss functions and custom loss functions (ref. pull request #22), I was made aware that usage of eval often is discouraged and can even be dangerous (if abused).

Therefore, I propose that we replace the three instances of it in this repository: model.py:

def get_model_instance(model_arch, model_params):
    try:
        model = eval(model_arch)

metric.py

def get_metric_functions(metric_names):
    try:
        metric_fns = [eval(metric) for metric in metric_names]

trainer.py

   def _valid_epoch(self, epoch):
        self.model.eval()

I am not certain about what to replace it with. Any of you know?

SunQpark commented 5 years ago

The third example model.eval() seems to have nothing to do with this issue. The eval() discouraged to use may refer to the first and second, which changes string into the variable handle. I don't understand how this can be dangerous but I felt this uncomfortable since the linter always complains about that. Maybe I can study this later. I will post some references here, if I find. I think we can just replace the eval() with use of dictionary of function handles.

SunQpark commented 5 years ago

From now on, it seems that simple usage of eval() just like ours has no problem. Here is reference

borgesa commented 5 years ago

Thanks, will look into that.

SunQpark commented 5 years ago

I have changed train.py to use neither eval(), wildcard import nor external get_something methods with use of getattr(). Closing this issue.