Add --target_dir option #83

moraval · 2020-04-01T09:52:48Z

I need to add tests.

coveralls · 2020-04-01T09:59:47Z

Pull Request Test Coverage Report for Build 4300

28 of 47 (59.57%) changed or added relevant lines in 8 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.04%) to 88.911%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
emloop/cli/resume.py	1	2	50.0%
emloop/cli/eval.py	0	2	0.0%
emloop/cli/train.py	1	3	33.33%
emloop/entry_point.py	0	3	0.0%
emloop/cli/args.py	0	11	0.0%

Files with Coverage Reduction	New Missed Lines	%
emloop/cli/args.py	1	8.33%
emloop/cli/eval.py	1	38.89%

Totals
Change from base Build 4154:	-0.04%
Covered Lines:	4017
Relevant Lines:	4518

💛 - Coveralls

blazekadam

please do not reformat the code randomly (or at least do it in a separate commit) it is quite hard to decipher the actual substance
the MainLoop shall be left out of this entirely
I think output_dir would be much better name for the argument since it would be in-line with output_root (and current in-code naming convention)
this should be tested

blazekadam · 2020-04-01T21:39:42Z

emloop/main_loop.py

@@ -32,6 +32,7 @@ class MainLoop(CaughtInterrupts):   # pylint: disable=too-many-instance-attribut
    def __init__(self,   # pylint: disable=too-many-arguments
                 model: AbstractModel, dataset: AbstractDataset,
                 hooks: Iterable[AbstractHook]=(),
+                 output_dir: str='',


blazekadam · 2020-04-01T21:40:08Z

emloop/main_loop.py

@@ -184,7 +186,8 @@ def _run_epoch(self, stream: StreamWrapper, train: bool) -> None:
                continue
            elif self._fixed_batch_size:
                if batch_sizes != {self._fixed_batch_size}:
-                    var, len_ = [(k, len(v)) for k, v in batch_input.items() if len(v) != self._fixed_batch_size][0]
+                    var, len_ = [(k, len(v)) for k, v in batch_input.items()


Please do not randomly reformat the code without changing its semantics

@blazekadam I would love to, but the problem is that I have an automatic pep8 style checker that updates the whole file if I save it..
But I'll try to avoid it as much as possible

Yea well you need to find some other way to check the code. Just see the update on line 198/199... :(

blazekadam · 2020-04-01T21:44:02Z

emloop/api.py

+    return output_dir
+
+
+def config_log_to_output_dir(config: dict, output_dir: str):


I truly can not comprehend this function name.

is the config arg even used?

what about

os.makedirs(x, exist_ok=True)

blazekadam · 2020-04-01T21:45:45Z

emloop/api.py

+    logging.info('\tOutput dir is: %s', output_dir)
+
+    if not os.path.exists(output_dir):
+        logging.info('\tOutput dir folder "%s" does not exist and will be created', output_dir)


Suggested change

logging.info('\tOutput dir folder "%s" does not exist and will be created', output_dir)

logging.info('\tOutput dir `%s` does not exist and will be created', output_dir)

blazekadam · 2020-04-01T21:47:40Z

emloop/api.py

    :param output_root: dir where output_dir shall be created
    :param restore_from: if not None, from whence the model should be restored (backend-specific information)

    :return: main loop object
    """
-    output_dir = dataset = model = hooks = main_loop = None
+    dataset = model = hooks = main_loop = None
+    output_dir = target_dir


This should warn you that something is fishy with the naming...

blazekadam · 2020-04-01T21:57:03Z

emloop/api.py


-    output_dir = create_output_dir(config=config, output_root=output_root)
+    if not output_dir:


Not sure if this is the only call of the create_output_dir. In any case, putting the whole use the given name or figure out one and make sure it exists logic to the already-existing create_output_dir function would be imo better and more compact. (And you would not have to think about the first sentence)

blazekadam · 2020-04-01T22:00:31Z

Hmmm... it looks like we do not test most of the cli sub-module. I would appreciate at least unit-testing the create_ouput_dir fn (a] generated name, b] specific name)

blazekadam · 2020-04-01T22:01:43Z

Ha, I ignored the WIP status of this PR... sorry. The remarks are quite valid though... :)

blazekadam · 2020-04-07T21:46:19Z

emloop/api.py

@@ -16,17 +16,24 @@
 from .main_loop import MainLoop


-def create_output_dir(config: dict, output_root: str, default_model_name: str='Unnamed') -> str:
+def create_output_dir(config: dict, output_root: str, default_model_name: str='Unnamed', output_dir: str='') -> str:


Lets have a None as the default.

emloop/api.py

blazekadam · 2020-04-07T21:51:08Z

emloop/api.py

    :return: path to the created output_dir
    """
+    if output_dir:
+        logging.info('\tOutput dir is: %s', output_dir)
+        os.makedirs(output_dir, exist_ok=True)


Suggested change

os.makedirs(output_dir, exist_ok=True)

os.makedirs(path.join(output_root, output_dir), exist_ok=True)

?

Well I expect the user to give the whole path to the --output_dir argument.. Not just the name of the output_dir within the output_root.

Obviously, but that is a bit confusing behavior. The --output_root argument is required (but have a default). I do not expect it to be entirely ignored if and only if --output_dir is present.

blazekadam · 2020-04-07T21:53:32Z

emloop/api.py

-    if not os.path.exists(output_dir):
-        logging.info('\tOutput dir folder "%s" does not exist and will be created', output_dir)
-        os.makedirs(output_dir)
+def create_config_log(config: dict, output_dir: str):


I still dislike this function (and its name). It does two quite different things none of which is what I would expect from the name (creating a log for logging the configuration?).

lets just rewrite the create_output_dir as

if output_dir is not None: # log that the dir was specified # join the output_root and outptu_dir to get output_path else: # log that the dir name will be generated # generate the name and join it to get the output_path # proceed with the dir creation, config dumping, logger set up etc.

and this fn is suddenly not needed

emloop/cli/train.py

blazekadam · 2020-04-10T21:53:28Z

emloop/api.py

@@ -16,7 +16,7 @@
 from .main_loop import MainLoop


-def create_output_dir(config: dict, output_root: str, default_model_name: str='Unnamed', output_dir: str='') -> str:
+def create_output_dir(config: dict, output_root: str, default_model_name: str='Unnamed', output_dir: str=None) -> str:


It is Optional[str] now

blazekadam · 2020-04-10T21:55:49Z

emloop/api.py

    :return: path to the created output_dir
    """
+    if output_dir:
+        logging.info('\tOutput dir is: %s', output_dir)
+        os.makedirs(output_dir, exist_ok=True)


Obviously, but that is a bit confusing behavior. The --output_root argument is required (but have a default). I do not expect it to be entirely ignored if and only if --output_dir is present.

emloop/api.py

blazekadam · 2020-04-10T21:57:52Z

emloop/api.py

    yaml_to_file(data=config, output_dir=output_dir, name=EL_CONFIG_FILE)

    # create file logger
    file_handler = logging.FileHandler(path.join(output_dir, EL_LOG_FILE))
    file_handler.setFormatter(logging.Formatter(EL_LOG_FORMAT, datefmt=EL_LOG_DATE_FORMAT))
    logging.getLogger().addHandler(file_handler)

+    logging.info(f'Output directory has name {output_dir}')


Suggested change

logging.info(f'Output directory has name {output_dir}')

logging.info(f'Created output directory with name {output_dir}')

blazekadam · 2020-04-10T21:58:19Z

emloop/api.py

@@ -224,7 +213,7 @@ def create_hooks(config: dict, model: Optional[AbstractModel]=None, dataset: Opt
    return hooks


-def create_main_loop(config: dict, output_root: str, restore_from: str=None, output_dir: str='') -> MainLoop:
+def create_main_loop(config: dict, output_root: str, restore_from: str=None, output_dir: str=None) -> MainLoop:


the arg is optional str now

blazekadam · 2020-04-16T08:55:54Z

pls resolve the conflicts and merge it, I ll re-review it afterwards

Co-Authored-By: Adam Blažek <adam.blazek@cognexa.com>

moraval · 2020-04-16T09:18:02Z

I need to push --force it, so that the commits won't be duplicate here - are you okay with it?

blazekadam

The code and tests look fine but I truly hate the whitespace changes :(

blazekadam · 2020-04-16T21:16:03Z

emloop/main_loop.py

@@ -184,7 +186,8 @@ def _run_epoch(self, stream: StreamWrapper, train: bool) -> None:
                continue
            elif self._fixed_batch_size:
                if batch_sizes != {self._fixed_batch_size}:
-                    var, len_ = [(k, len(v)) for k, v in batch_input.items() if len(v) != self._fixed_batch_size][0]
+                    var, len_ = [(k, len(v)) for k, v in batch_input.items()


Yea well you need to find some other way to check the code. Just see the update on line 198/199... :(

blazekadam requested changes Apr 1, 2020

View reviewed changes

moraval changed the title ~~WIP: Add --target_dir option~~ Add --target_dir option Apr 6, 2020

blazekadam requested changes Apr 7, 2020

View reviewed changes

blazekadam requested changes Apr 10, 2020

View reviewed changes

moraval and others added 8 commits April 16, 2020 11:02

Add --target_dir option

bb32c12

Better name and place

6ebf94f

Add test

f4e0015

Apply suggestions from code review

e1d8e00

Co-Authored-By: Adam Blažek <adam.blazek@cognexa.com>

Merge two functions into one

0bd05b4

Correct bug

e125c7a

Create output_dir in output_root

28167d4

Correct test

6da599e

After merge cleanup

7c38c4b

moraval force-pushed the target-dir-option branch from 8598e1c to 7c38c4b Compare April 16, 2020 13:24

Update emloop/api.py

906d67d

blazekadam approved these changes Apr 16, 2020

View reviewed changes

moraval merged commit 7596b05 into dev Apr 17, 2020

moraval deleted the target-dir-option branch April 17, 2020 09:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --target_dir option #83

Add --target_dir option #83

moraval commented Apr 1, 2020

coveralls commented Apr 1, 2020 •

edited

Loading

blazekadam left a comment •

edited

Loading

blazekadam Apr 1, 2020

blazekadam Apr 1, 2020

moraval Apr 3, 2020 •

edited

Loading

blazekadam Apr 16, 2020

blazekadam Apr 1, 2020

blazekadam Apr 1, 2020

blazekadam Apr 1, 2020

blazekadam Apr 1, 2020

blazekadam commented Apr 1, 2020

blazekadam commented Apr 1, 2020

blazekadam Apr 7, 2020

blazekadam Apr 7, 2020

moraval Apr 8, 2020

blazekadam Apr 10, 2020

blazekadam Apr 7, 2020

blazekadam Apr 10, 2020 •

edited

Loading

blazekadam Apr 10, 2020

blazekadam Apr 10, 2020

blazekadam Apr 10, 2020 •

edited

Loading

blazekadam commented Apr 16, 2020

moraval commented Apr 16, 2020

blazekadam left a comment

blazekadam Apr 16, 2020

		return output_dir


		def config_log_to_output_dir(config: dict, output_dir: str):

	logging.info('\tOutput dir folder "%s" does not exist and will be created', output_dir)
	logging.info('\tOutput dir `%s` does not exist and will be created', output_dir)


		output_dir = create_output_dir(config=config, output_root=output_root)
		if not output_dir:

	os.makedirs(output_dir, exist_ok=True)
	os.makedirs(path.join(output_root, output_dir), exist_ok=True)

	logging.info(f'Output directory has name {output_dir}')
	logging.info(f'Created output directory with name {output_dir}')

Add --target_dir option #83

Add --target_dir option #83

Conversation

moraval commented Apr 1, 2020

coveralls commented Apr 1, 2020 • edited Loading

Pull Request Test Coverage Report for Build 4300

💛 - Coveralls

blazekadam left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moraval Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazekadam commented Apr 1, 2020

blazekadam commented Apr 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazekadam Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazekadam Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

blazekadam commented Apr 16, 2020

moraval commented Apr 16, 2020

blazekadam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 1, 2020 •

edited

Loading

blazekadam left a comment •

edited

Loading

moraval Apr 3, 2020 •

edited

Loading

blazekadam Apr 10, 2020 •

edited

Loading

blazekadam Apr 10, 2020 •

edited

Loading