pytorch-ignite / code-generator

Web Application to generate your training scripts with PyTorch Ignite
https://code-generator.pytorch-ignite.ai/
BSD 3-Clause "New" or "Revised" License
40 stars 23 forks source link

Separate the output_dir and sub_output_dir to make reproducing results easier #306

Closed guptaaryan16 closed 1 year ago

guptaaryan16 commented 1 year ago

Description

This PR introduces a distinction between output_dir and sub_output_dir in the templates. This simplifies the end result for the user to access the logs/<job-dir> and integration of other reproducibility based features for templates in future.

Fix #292

Additional context


What is the purpose of this pull request?

netlify[bot] commented 1 year ago

Deploy Preview for code-generator ready!

Name Link
Latest commit 5d20e6e97f0f59a32a91f9e730ce66c827dcb10d
Latest deploy log https://app.netlify.com/sites/code-generator/deploys/64ed9b3894c94c00070ec143
Deploy Preview https://deploy-preview-306--code-generator.netlify.app/
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

vfdev-5 commented 1 year ago

@guptaaryan16 I'm not very fan of introducing this param which is very close to output_dir. Without any description of what it does it leads to confusion.

guptaaryan16 commented 1 year ago

@vfdev-5 The thing is we need to separate the job-dir from output-dir: ./logs so that using job-dir becomes a bit simplified. This makes it a bit easier to access the logging and checkpointing of each run and removes limitations for integration of other libraries

vfdev-5 commented 1 year ago

Can we do simply do the following instead of introducing another param ?

    output_dir = setup_output_dir(config, rank)
    if rank == 0:
        save_config(config, output_dir)
    config.output_dir = output_dir
guptaaryan16 commented 1 year ago

The function definition of setup_output_dir requires to do it in a way such that if its a subprocess, it should return just config.output_dir variable which seems difficult to integrate with the one variable fix

def setup_output_dir(config: Any, rank: int) -> Path:
    if rank == 0:
        now = datetime.now().strftime("%Y%m%d-%H%M%S")
        name = f"{now}-backend-{config.backend}-lr-{config.lr}"
        path = Path(config.output_dir, name)
        path.mkdir(parents=True, exist_ok=True)
        config.output_dir = path.as_posix()
    return Path(idist.broadcast(config.output_dir, src=0))

If we want to do this we have to change this function as

def setup_output_dir(config: Any, rank: int) -> Path:
    """Create output folder."""
    if rank == 0:
        now = datetime.now().strftime("%Y%m%d-%H%M%S")
        name = f"{now}-backend-{config.backend}-lr-{config.lr}"
        path = Path(config.output_dir, name)
        path.mkdir(parents=True, exist_ok=True)
        output_dir = path.as_posix()
        return Path(idist.broadcast(output_dir, src=0))
    return Path(idist.broadcast(config.output_dir, src=0))

Does this seem right to you

vfdev-5 commented 1 year ago

OK, let's update it to:

def setup_output_dir(config: Any, rank: int) -> Path:
    output_dir = config.output_dir
    if rank == 0:
        now = datetime.now().strftime("%Y%m%d-%H%M%S")
        name = f"{now}-backend-{config.backend}-lr-{config.lr}"
        path = Path(config.output_dir, name)
        path.mkdir(parents=True, exist_ok=True)
        output_dir = path.as_posix()
    return Path(idist.broadcast(output_dir, src=0))