Media-Smart / vedadet

A single stage object detection toolbox based on PyTorch
Apache License 2.0
498 stars 128 forks source link

[Possible Bug Fix]Making Config Class OS Agnostic #57

Closed Ehsan1997 closed 3 years ago

Ehsan1997 commented 3 years ago

Hi, First of all thanks for creating this wonderful project. I plan to use some parts of your project for my work. While studying the code in your project I realized that your Config class makes use of tempfile and shutil simultaneously, which causes a problem in windows software.

The problem arises when you create the tempfile using NamedTemporaryFile and then use shutils.copyfile. Once the temporary file is created it is opened automatically, then when you use shutils.copyfile, it tries to open the file again and windows doesn't allow it. So an easy workaround would be to close automatic deletion of the file and close the file before copying stuff into it. Later you can reopen the file and finally close and manually delete it.

So on high level:

temp_file = NamedTemporaryFile(delete=False)
temp_file.close()
shutil.copyfile(src_file, temp_file)
temp_file = open(temp_file_path)
# Code Goes Here
temp_file.close()
os.unlink

I will mention the complete modified _file2dict method here for reference:

    def _file2dict(filename):
        filename = osp.abspath(osp.expanduser(filename))
        check_file_exist(filename)
        if filename.endswith('.py'):
            with tempfile.TemporaryDirectory() as temp_config_dir:
                temp_config_file = tempfile.NamedTemporaryFile(
                    dir=temp_config_dir, suffix='.py', delete=False)
                temp_config_file_name = temp_config_file.name
                temp_config_name = osp.basename(temp_config_file_name)
                temp_config_file.close()
                shutil.copyfile(filename,
                                osp.join(temp_config_dir, temp_config_name))
                temp_config_file = open(temp_config_file_name, 'a')
                temp_module_name = osp.splitext(temp_config_name)[0]
                sys.path.insert(0, temp_config_dir)
                Config._validate_py_syntax(filename)
                mod = import_module(temp_module_name)
                sys.path.pop(0)
                cfg_dict = {
                    name: value
                    for name, value in mod.__dict__.items()
                    if not name.startswith('__')
                }
                # delete imported module
                del sys.modules[temp_module_name]
                # close temp file
                temp_config_file.close()
                os.unlink(temp_config_file_name)
        elif filename.endswith(('.yml', '.yaml', '.json')):
            from .. import fileio
            cfg_dict = fileio.load(filename)
        else:
            raise IOError('Only py/yml/yaml/json type are supported now!')

        cfg_text = filename + '\n'
        with open(filename, 'r') as f:
            cfg_text += f.read()

        if BASE_KEY in cfg_dict:
            cfg_dir = osp.dirname(filename)
            base_filename = cfg_dict.pop(BASE_KEY)
            base_filename = base_filename if isinstance(
                base_filename, list) else [base_filename]

            cfg_dict_list = list()
            cfg_text_list = list()
            for f in base_filename:
                _cfg_dict, _cfg_text = Config._file2dict(osp.join(cfg_dir, f))
                cfg_dict_list.append(_cfg_dict)
                cfg_text_list.append(_cfg_text)

            base_cfg_dict = dict()
            for c in cfg_dict_list:
                if len(base_cfg_dict.keys() & c.keys()) > 0:
                    raise KeyError('Duplicate key is not allowed among bases')
                base_cfg_dict.update(c)

            base_cfg_dict = Config._merge_a_into_b(cfg_dict, base_cfg_dict)
            cfg_dict = base_cfg_dict

            # merge cfg_text
            cfg_text_list.append(cfg_text)
            cfg_text = '\n'.join(cfg_text_list)

        return cfg_dict, cfg_text

P.S - I apologize for not creating a proper pull request, my schedule is a bit tight these days.

hxcai commented 3 years ago

@Ehsan1997 Thanks for your suggestion, and we will check the problem.