Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

guptaaryan16
Copy link
Contributor

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?

  • Bug fix
  • New feature
  • Other

@netlify
Copy link

netlify bot commented Aug 29, 2023

Deploy Preview for code-generator ready!

Name Link
🔨 Latest commit 5d20e6e
🔍 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.

@guptaaryan16 guptaaryan16 changed the title Separate the output_dir and sub_output_dir to fix bugs for reproducing results Separate the output_dir and sub_output_dir to make reproducing results easier Aug 29, 2023
@vfdev-5
Copy link
Member

vfdev-5 commented Aug 29, 2023

@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 guptaaryan16 marked this pull request as draft August 29, 2023 07:31
@guptaaryan16
Copy link
Contributor Author

@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
Copy link
Member

vfdev-5 commented Aug 29, 2023

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
Copy link
Contributor Author

guptaaryan16 commented Aug 29, 2023

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
Copy link
Member

vfdev-5 commented Aug 29, 2023

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))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants