automl / NASLib

NASLib is a Neural Architecture Search (NAS) library for facilitating NAS research for the community by providing interfaces to several state-of-the-art NAS search spaces and optimizers.
Apache License 2.0
528 stars 117 forks source link

fixed config not read issue #30

Closed kashankrm closed 3 years ago

kashankrm commented 3 years ago

Issue: when called get_config_from_args with ''--config-file" option it does not load that config file. Solution: I noticed that args variable was supposed to be parsed but wasn't so i created the parser and parsed it. at line 170 I was getting filenotfound error so I added project root with the config_file which fixed the issue.

Jabb0 commented 3 years ago

I already wondered how the args parameter should be used. Note sure if this is the correct usage as it states that the input can be from a different argparser without specifiying the dataformat. Based on how it is used the args parameter should be a dict. I think it is possible to modularize this config parsing more for more easier programmatic control.

The file not found fix does not work if the path given is already absolute. I recommend using the old way because it is relative to the path where the python command has been issued.

kashankrm commented 3 years ago

I am actually taking DLlab21 practical course where this issue was discussed and Mr. Zela said that these changes were correct. In one example (https://github.com/automl/NASLib/blob/dllab21/docs/naslib_tutorial.ipynb section NAS Predictors ) you can see that there is a function call like this config = utils.get_config_from_args(args=["--config-file", "naslib/benchmarks/predictor_config.yaml"], config_type="predictor") here args is a list of str not dict so I thought it needs to parsed which is why I just used the default parser. But perhaps he only meant that it was correct inside the context of that course. And I see that in the docstring it says "args: args from a different argument parser than the default one." which further adds more confusion. Anyways you are correct that maybe there needs to be more functionality in argument parsing, maybe we can parse with default parser if args is a list of str and do nothing if its a dict? and maybe we can also add an extra parameter for custom parser that is mentioned in doc string such that if provided we will just use that to parse our list of str otherwise just use default one?

and about the relative file thing you are absolutely correct It should be reverted the old version. I will wait for your response then make the changes and push. Regards, Kashan

kashankrm commented 3 years ago

Oh I just checked he updated this file in this commit https://github.com/automl/NASLib/commit/0c17d4079322540009c027a3969afba738150b2e I think I should close this since the original issue is fixed, but feel free to reopen if you think there should be further changes. Regards, Kashan