Skip to content
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

WIP: Non-Interactive Docker Run #5

Merged
merged 10 commits into from
Jan 29, 2025

Conversation

iksnagreb
Copy link

When running some automated finn builds by some experiment manager doing parameter sweeps, the jobs failed with some TTY issues from within docker, which I have tracked down to the container being connected to a non-TTY output device. So I think there should be an option to disable DOCKER_INTERACTIVE, when it is not absolutely needed. This PR is certainly not ready (it just disables DOCKER_INTERACTIVE...). This is more of reminder, to myself that I rely on this being fixed...

This needs to be properly refactored and probably be exposed to the outside so the user is in control. Maybe default to interactive runs while introducing some flag to turn this off?

See Xilinx#967

iksnagreb and others added 7 commits April 3, 2024 15:09
This needs to be properly refactored and probably be exposed to the
outside so the user is in control. Maybe default to interactive runs
while introducing some flag to turn this off?
@bwintermann
Copy link

Additionally to this change here, we also have a new feature branch feature/poetry_setup which aims to remove the Docker based setup completely or to at least make it optional. Maybe this would also fix your issue here?

@iksnagreb
Copy link
Author

Yes, getting rid of the whole docker setup would be ideal in the long term. This is more like a workaround... This also sometimes messes up the logging/console output, that is why it is a draft...

@fpjentzsch
Copy link

This doesn't affect the Singularity setup, so it is fine for me if you implement a user-controllable DOCKER_INTERACTIVE that defaults to the old behavior (letting people use the Container interactively).

@iksnagreb iksnagreb self-assigned this Jan 28, 2025
Tests, notebooks and build flows now default to non-interactive docker
run while while the container-only option defaults to the interactive
bash prompt just as before.
@iksnagreb
Copy link
Author

I have just updated the defaults: Tests, notebooks (hosting the server) and build flows now default to non-interactive docker run while the container-only option defaults to the interactive bash prompt just as before. Is this fine for everyone?

@iksnagreb iksnagreb marked this pull request as ready for review January 29, 2025 12:23
Copy link
Member

@LinusJungemann LinusJungemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@fpjentzsch
Copy link

fpjentzsch commented Jan 29, 2025

Doesn't this break the integrated interactive pdb debugging that launches by default when the builder fails?

enable_build_pdb_debug: Optional[bool] = True

@iksnagreb
Copy link
Author

iksnagreb commented Jan 29, 2025

Hm, yes, probably, so we should change the default to always be interactive? Or disable the pdb by default? What actually happens when some CI or experiment job (maybe even on some cluster node which you are not even observing with some interactive terminal) drops you into this pdb session? Does it wait and hang-up your job until timeout or does it crash immediately as there is no input attached?

@LinusJungemann
Copy link
Member

Jobs on the cluster will directly end the pdb automatically and exit the job

@fpjentzsch
Copy link

Yes, but I would still default to the old behavior as in finn/dev for maximum compatibility. Disabling DOCKER_INTERACTIVE is only relevant for your dvc setup, so you can simply overwrite it with an env var on your end.

@fpjentzsch fpjentzsch merged commit 1693f2a into eki-project:dev Jan 29, 2025
1 check passed
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.

6 participants