Skip to content

Make heatconduction thread safe#198

Merged
g-braeunlich merged 2 commits into
mainfrom
container
Oct 10, 2025
Merged

Make heatconduction thread safe#198
g-braeunlich merged 2 commits into
mainfrom
container

Conversation

@g-braeunlich
Copy link
Copy Markdown
Collaborator

Description

  • Support for stdin in utils.container
  • Use stdin and args to pass data to the scripts running inside the container instead of using files.
  • Add os.getpid() as a suffix for the output files generated by the scripts runnsing inside the container to prevent overwrites

Probably related to: #193
Part of: #191

Type of change

Please delete options that are not relevant.

  • Documentation only change (no code changed)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

@g-braeunlich g-braeunlich self-assigned this Sep 30, 2025
@g-braeunlich g-braeunlich marked this pull request as draft September 30, 2025 14:16
@g-braeunlich
Copy link
Copy Markdown
Collaborator Author

For now this is only 2d. Also I still left the original behavior (reading from files) in case this is still needed.
If not, I am happy to remove the code.

Copy link
Copy Markdown
Collaborator

@ffelten ffelten left a comment

Choose a reason for hiding this comment

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

Simple and working, the kind of solutions I like

Comment thread engibench/problems/heatconduction2d/templates/optimize_heat_conduction_2d.py Outdated
np.save(serialized_design, design)

current_dir = os.getcwd()
output_path = f"templates/RES_SIM/Performance-{os.getpid()}.txt"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👉 This could be a nice pattern for file creation in general

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Note also #46 which we might solve as well

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I changed the path pattern a bit.
It is now: templates/output-by-pid/{os.getpid()}/{original_filename}.
This way, I can reuse the pid directory for other intermediate files

@g-braeunlich
Copy link
Copy Markdown
Collaborator Author

3d is also done now.
I extracted some code to engibench.utils.cli and engibench.problems.heatconduction2d.shared.

@g-braeunlich g-braeunlich marked this pull request as ready for review October 10, 2025 06:59
@g-braeunlich g-braeunlich requested a review from ffelten October 10, 2025 07:00
@g-braeunlich
Copy link
Copy Markdown
Collaborator Author

@ffelten As I changed a couple of things since your last review, could I ask you again to have a look before I merge?

Copy link
Copy Markdown
Collaborator

@ffelten ffelten left a comment

Choose a reason for hiding this comment

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

A few minor things. Thanks for cleaning this stuff up, it felt good reading this PR

# Load initial design image
image = np.load(input_path)
os.remove(input_path) # Remove after loading
image = np_array_from_stdin()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would read all the inputs at the beginning of the script instead

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

# Load initial design image
image = np.load(input_path)
os.remove(input_path) # Remove after loading
image = np_array_from_stdin()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same here

cli_module_path = cli.__file__

# Copy required stuff to a templates subfolder.
# We cannot just mount, as the engibench folder might not be a subfolder of
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Indeed, was about to ask if you tried to pip install and run this from another directory?

],
stdin=stdin,
)
return load_output(output_path)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not super fan of passing the load_output function here, it would be clearer at call site if the caller receives the output path and loads it himself, no?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

To me the load_output argument is like a choice of the format. But yes, I agree that this would disentangle the execution flow a bit

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done now

@g-braeunlich g-braeunlich merged commit ea5f88d into main Oct 10, 2025
13 checks passed
@g-braeunlich g-braeunlich deleted the container branch October 10, 2025 16:10
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