Implement changes to make low level splishsplash simulation API call more usable #23

luispcunha · 2023-02-24T17:33:53Z

In this PR, the suggestions made by @fabiocruz are followed in order to make the run_simulation method more user-friendly.
The changes bowl down to:

allow specifying a different directory for the outputs.
having a better type annotation for the input of the method.

Closes #33.

luispcunha · 2023-02-28T12:37:20Z

@begaiym-k @sarmento @hpenedones I add the simulation class for splish splash. Check the demos/splishasplash_execution.py file to see what the user interaction looks like. Wdyt?

begaiym-k · 2023-02-28T15:09:18Z

demos/splishsplash_simulation.py

+flags.DEFINE_string("output_dir",
+                    None,
+                    "Directory where the outputs will be stored.",
+                    required=True)


I would suggest using flags.mark_flags_as_required instead.

What is the difference?

You will not have to set required inside each flag, and it visually looks better.

I prefer using the argument required=True in each flag. IMO it's easier to read since the information about a flag being required or not is closer to its name, description, default value, etc.

begaiym-k · 2023-02-28T15:11:10Z

demos/splishsplash_simulation.py

+                    None,
+                    "Directory with the simulation inputs.",
+                    required=True)
+flags.DEFINE_string("input_file", None, "Input file.", required=True)


Are we not assuming that the input file is inside the sim_dir directory already?

Yes, this is actually to tell the name of the file inside the directory. I will make that more explicit. I think it is better to not constrain the user to use a specific filename and allow the user to specify the name of the input file.

Got it. Yes, a more explicit describtion is needed.

begaiym-k · 2023-02-28T15:18:59Z

inductiva/fluids/scenarios/dam_break.py

-        sim_output_path = inductiva.sph.run_simulation(
-            DirPath(input_temp_dir.name))
-        simulation._output_directory = sim_output_path.path  #pylint: disable=protected-access
+        sim_output_path = inductiva.sph.splishsplash.run_simulation(


I think we can add simulation output directory as an optional parameter, wdyt?

to .simulate() ?
yes, I think it is a good idea.

fabiocruz

LGTM! I have minor-ish comments only.

demos/dambreak_simulation.py

fabiocruz · 2023-03-01T10:51:49Z

demos/splishsplash_simulation.py

+flags.DEFINE_string("output_dir",
+                    None,
+                    "Directory where the outputs will be stored.",
+                    required=True)


I prefer using the argument required=True in each flag. IMO it's easier to read since the information about a flag being required or not is closer to its name, description, default value, etc.

demos/splishsplash_simulation.py

inductiva/fluids/scenarios/dam_break.py

inductiva/fluids/splishsplash.py

fabiocruz · 2023-03-01T10:57:30Z

inductiva/fluids/splishsplash.py

+            directory.
+    """
+
+    def __init__(self, sim_dir: Path, input_file_name: str):


IMO input_file_name (or input_filename) should have a default value, e.g. splishsplash_input.json?

I added that default value to run_simulation. Tbh I think it is a bit weird to have a default value for a file name that is provided by the user. If the user provides the file, then it should explicitly specify the name.
There wouldn't be an intuitive and self-contained way inside the code to tell the user that he should name the input file "splishsplash_input.json".

I do think though that it is equally weird to have a default value here and on run_simulation... Not having a default value here will force the user (e.g. me ahah) to always write input_filename="splishsplash_input.json".

But that's ok, we can work without a default value here.

i added the default value

🙏 Thank you!

inductiva/sph/splishsplash.py

inductiva/utils/data.py

fabiocruz · 2023-03-01T11:02:17Z

inductiva/utils/data.py

+    if var_type == pathlib.Path:
+        return pathlib.Path(os.path.join(output_dir, value))


Why are we checking some types against inductiva.types.Path and others against pathlib.Path? Shouldn't we try to use only one type do describe paths?

My perspective is:

input types: be as broad as possible, so that the user is not constrained to use pathlib.Path and can also use strings or other things.

output types: be as specific as possible, so that the user knows exactly what it receives (receiving a pathlib.Path is different than receiving a str)

Good point. Let's write it in some documentation in the future, maybe when writing #34?

fabiocruz · 2023-03-01T11:03:47Z

demos/splishsplash_simulation.py

+                    None,
+                    "Directory with the simulation inputs.",
+                    required=True)
+flags.DEFINE_string("input_file_name",


I think we should name this input_filename, for consistency with the output_filename used in other files of the package.

That would introduce changes in the other repository, do you think it's worth it?

Yep, consistency is definitely worth it! It saves us (or at least it saves me...) a lot of time when reading code.

https://github.com/inductiva/inductiva-web-api/pull/45

hpenedones · 2023-03-01T12:52:03Z

inductiva/types.py

 import os
+from typing import TypeAlias, Union



Ok, I come later to the party, sorry!
Good that we deleted this DirPath class, because:

why Inductiva would need to define its own classes to deal with paths? Are we different from the millions of python developers in the world, with respect to paths to files or folders?

I would go a step further and question the existence of this types.py file ... even though it's now simpler because it is just a TypeAlias , the same questions applies: why did the millions of developers dealing with python, filepaths etc, use the standard types and we need to define a new one?

If it is to solve an implementation issue downstream, probably that means that the mechanism downstream needs to be written in a different manner with different assumptions, but it can't work based on the fact that we defined a new Path type...

There is no standard type for Paths (https://peps.python.org/pep-0519/#provide-specific-type-hinting-support). If you look into the type annotation of open(), it is also a complicated TypeAlias with a Union of a bunch of types.

Fábio (the user) wanted to be able to pass the path as both a string, or as a pathlib.Path, so creating a typealias in order to not write a union everytime is better.

But yes, downstream we also use the type annotation in order to know how to pack the data correctly. we can thing of a different of doing that...

Thanks @luispcunha for the additional context, you can push back on the user request from @fabiocruz ! :) We are not yet able to run simulations in a stable manner and efficiently, and there are many P0 requests, do we believe that supporting multiple types to specify paths is a priority and it justifies increasing the code complexity to the point of creating new modules / files ??

@fabiocruz : next time please open feature request as an issue and we will prioritize it on our meetings?

When this was done that system still wasn't in place.
That will still be a problem, because right now we rely on the type annotation to know that the argument is a directory and that we need to include that directory in the zip that we send to the API. So just removing this doesn't solve the issue.
A possible solution would be to use pathlib.Path as the type, and then checking if the path points to a directory. Wdyt?

Thanks @luispcunha for the additional context, you can push back on the user request from @fabiocruz ! :) We are not yet able to run simulations in a stable manner and efficiently, and there are many P0 requests, do we believe that supporting multiple types to specify paths is a priority and it justifies increasing the code complexity to the point of creating new modules / files ??

@fabiocruz : next time please open feature request as an issue and we will prioritize it on our meetings?

@hpenedones this came from the prioritization defined in last week's meeting. I was supposed to identify how we could launch SPH simulations with a low-level interface and user pain points. I did that and coordinated with @luispcunha and @begaiym-k to implement the required changes.

Sorry that we didn't create an issue and discuss it before implementation took place. Issues are a recent practice here :-) Will document feature requests better from now on.

luispcunha added 7 commits February 23, 2023 18:10

Add modifiable output dir and change typing of run_simulation

6882463

Fix output dir of run simulation

4f748b1

Add docstring

b05399e

Add modifiable output dir and change typing of run_simulation

2dfb847

Fix output dir of run simulation

c69515f

Add docstring

a40dd8b

Fix merge

5b7da64

luispcunha requested review from hpenedones, sarmento, fabiocruz and begaiym-k February 24, 2023 17:33

luispcunha added 3 commits February 28, 2023 12:28

Add splishsplash simulation class

23b7e1a

Add ignore missing-module-docstring to some __init__ files

9aff9e4

Lint

9a0ed8f

begaiym-k approved these changes Feb 28, 2023

View reviewed changes

Rename input_file to input_file_name in splishsplash simulation

47bb519

fabiocruz approved these changes Mar 1, 2023

View reviewed changes

luispcunha added 4 commits March 1, 2023 11:20

Minor changes to comments and styling

bb47557

input_file_name to input_filename

b97e4a1

Add default value to input_filename

5118b37

Merge branch 'main' into lpc-improve-run-sim

d83013c

luispcunha merged commit 2dc56f9 into main Mar 1, 2023

luispcunha deleted the lpc-improve-run-sim branch March 1, 2023 12:15

hpenedones reviewed Mar 1, 2023

View reviewed changes

fabiocruz mentioned this pull request Mar 1, 2023

Requirements for generating Fluid Cube dataset #52

Closed

6 tasks

hpenedones mentioned this pull request Mar 1, 2023

Argument type used to identify that directory needs to be uploaded #53

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement changes to make low level splishsplash simulation API call more usable #23

Implement changes to make low level splishsplash simulation API call more usable #23

luispcunha commented Feb 24, 2023 •

edited

Loading

luispcunha commented Feb 28, 2023

begaiym-k Feb 28, 2023

luispcunha Feb 28, 2023

begaiym-k Feb 28, 2023

fabiocruz Mar 1, 2023

begaiym-k Feb 28, 2023

luispcunha Feb 28, 2023

begaiym-k Feb 28, 2023

begaiym-k Feb 28, 2023

luispcunha Feb 28, 2023

fabiocruz left a comment

fabiocruz Mar 1, 2023

fabiocruz Mar 1, 2023

luispcunha Mar 1, 2023

fabiocruz Mar 1, 2023

luispcunha Mar 1, 2023

fabiocruz Mar 1, 2023

fabiocruz Mar 1, 2023

luispcunha Mar 1, 2023

fabiocruz Mar 1, 2023

fabiocruz Mar 1, 2023

luispcunha Mar 1, 2023

fabiocruz Mar 1, 2023

luispcunha Mar 1, 2023

hpenedones Mar 1, 2023

luispcunha Mar 1, 2023

hpenedones Mar 1, 2023

luispcunha Mar 1, 2023

fabiocruz Mar 1, 2023

		if var_type == pathlib.Path:
		return pathlib.Path(os.path.join(output_dir, value))

Implement changes to make low level splishsplash simulation API call more usable #23

Implement changes to make low level splishsplash simulation API call more usable #23

Conversation

luispcunha commented Feb 24, 2023 • edited Loading

luispcunha commented Feb 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiocruz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luispcunha commented Feb 24, 2023 •

edited

Loading