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

issue a warning when to_sdfg() ignores auto_optimize flag #1380

Closed
rohanrayan opened this issue Oct 3, 2023 · 6 comments · Fixed by #1395
Closed

issue a warning when to_sdfg() ignores auto_optimize flag #1380

rohanrayan opened this issue Oct 3, 2023 · 6 comments · Fixed by #1395

Comments

@rohanrayan
Copy link

Describe the bug
dace to issue a warning when to_sdfg() ignores the auto_optimize flag

To Reproduce
I am not sure if this is a bug or a misunderstanding from my part, I have tested this on v0.14.4 of dace.
I have a simple function below:

@dace.program(auto_optimize=True)
def madd(A, B, m, n):
for i in range(0, m):
for j in range(0, n):
A[i][j] += B[i][j]
return A

I define example numpy matrices as follows:

m = 200
n = 250
vA = np.random.rand(m, n)
vB = np.random.rand(m, n)

When I try to generate the SDFG using the following commands:

madd_dace_sdfg = madd.to_sdfg(vA, vB, m, n)
madd_dace_compiled = madd_dace_sdfg.compile()

I see that the auto_optimize flag for the function gets ignored i.e. it produces the same graph regardless of whether the auto_optimize flag is set to True or False.

On the other hand, if I generate the graph using the following command (without going through the intermediate to_sdfg():

madd_dace_compiled = madd.compile(vA, vB, m, n)

I see that the graph changes based on the auto_optimize flag.

Is there something I am missing?

Thanks in advance!

@BenWeber42
Copy link
Contributor

Hi, thanks for the bug report, good catch 👍
I will try to look into this soonish (currently still working on other issues).

@philip-paul-mueller
Copy link
Collaborator

I will take care of this, as my first assignment.

@philip-paul-mueller
Copy link
Collaborator

I am not fully sure if it is a bug or just unexpected behaviour.
The compile() function of the DaceProgram (dace/frontend/python/parser.py:280) will respect the flag, while to_sdfg() will just return the SDFG as it is.
I think that the original idea was that if you request the SDFG you want to modify the graph yourself.
Which make sense but is a bit unintuitive.

I see two solutions for this:

  1. Issue a warning that the SDFG is not optimized. The advantage of this is, that it for sure will not break any code.
  2. Optimize the SDFG before it is returned, which would be intuitive, but might break legacy code.

In my opinion we should adopt the second option, since it is intuitive.
And it is very unexpected that self.compile() should differ from self.to_sdfg().compile().
However, for certain cases there should be a flag to suppress the auto optimization.

I will soon^{(R)} made a suggestion.

@rohanrayan
@BenWeber42

@philip-paul-mueller
Copy link
Collaborator

However, what should the __sdfg__() function do? I think it should behave in the same way as to_sdfg() does.

@alexnick83
Copy link
Contributor

There isn't a bug here, however, the workflow is unclear and inconsistent.

The decorator's auto_optimize parameter is supposed to take effect when directly calling the DaCeProgram itself or when (again directly) compiling it. As @philip-paul-mueller noted, when converting a DaCeProgram to SDFG, it is expected that the user wants to modify the graph by themselves. However, the to_sdfg method will obey the automatic_simplification configuration variable. There may be an inconsistency with the autooptimize configuration flag and what happens with nested SDFGs, but I am not sure.

My suggestion would be to (1) write down what exactly happens with all the different ways leading to a CompiledSDFG (including having a nested SDFG, perhaps by calling another DaCeProgram), (2) discuss in the DaCe weekly meeting whether these workflows need amendments, and (3) update the documentation to provide clarifications to the users.

@philip-paul-mueller
Copy link
Collaborator

After looking a bit in the issue I think the best thing we could do is output a warning as it was originally suggested by @rohanrayan. But I will bring this up at the next meeting.

philip-paul-mueller pushed a commit to philip-paul-mueller/dace that referenced this issue Oct 16, 2023
As it was discussed the ignoring of the `auto_optimize` flag in `to_sdfg()` is intentional. To make it clear to the user the function now issues a warning in this case.
BenWeber42 pushed a commit that referenced this issue Nov 14, 2023
…#1380). (#1395)

As it was discussed the ignoring of the `auto_optimize` flag in
`to_sdfg()` is intentional. To make it clear to the user the function
now issues a warning in this case.

---------

Co-authored-by: Philip Mueller, KY <philip.paul.mueller@bluemail.ch>
Co-authored-by: Tal Ben-Nun <tbennun@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants