Skip to content

Conversation

@joasode
Copy link
Contributor

@joasode joasode commented Jul 4, 2025

No description provided.

@joasode joasode marked this pull request as ready for review July 4, 2025 08:31
@Chroxvi
Copy link
Contributor

Chroxvi commented Jul 4, 2025

I like this idea. I think it provides a significant improvement over the existing error handling. However, I would like to propose a slightly different implementation strategy:

  • Use the cotainr.container.logger for logging the FATAL message instead of the subprocess logger.
    • Get the verbosity level from SingularitySandbox._verbosity.
  • Instead of directly calling sys.exit(0) directly in the container module, raise a RuntimeError from the CalledProcessError and catch it in the relevant place in the cli module, then call sys.exit(0) there as the handling of the RuntimeError. (Calling sys.exit(0) outside of the cli module is a bit brutal since people may (theoretically) what to just use cotainr as a library and build their own user interface for it.) (Writing this, I realize that we may have a problem here:
    sys.exit(0)
    )

Also we might consider:

  • Adding extra cotainr DEBUG messages around this call to the subprocess to:
    • make it easier to identify where we are in the cotainr code base when the problem occurs
    • record the value of some of the key variables we are passing around in cotainr related to the subprocess
  • Including the cmd in the FATAL log text even when verbosity<4.

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.

3 participants