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

disable __setitem__ in static mode & add API paddle.static.setitem with dy2st strategy #53682

Merged
merged 33 commits into from
Jul 19, 2023

Conversation

zoooo0820
Copy link
Contributor

@zoooo0820 zoooo0820 commented May 10, 2023

PR types

Breaking changes

PR changes

APIs

Description

Pcard-66985

Motivation

Currently, the __setitem__ in static mode is implemented by directly overwriting the result on the original Variable. This leads to although the forward result is correct, the calculation result is wrong in the backward stage (some are because result relies on the initial value of this Variable; some are because the graph in backward constructed according to this also has the problem of overwriting ).

# Dygraph is right
>>> a = paddle.full((2,3,4), 1.2, dtype='float32')
>>> a.stop_gradient=False
>>> v = paddle.ones((1,), dtype='float32')
>>> v.stop_gradient=False
>>> b = a * 2
>>> b[0,:, 2] = b[0, :, 2] * v
>>> l = b.mean()
>>> l.backward()
>>> b.sum()
Tensor(shape=[], dtype=float32, place=Place(gpu:0), stop_gradient=False,
       57.60000229)
>>> a.grad.sum()
Tensor(shape=[], dtype=float32, place=Place(gpu:0), stop_gradient=False,
       2.)
>>> v.grad
Tensor(shape=[1], dtype=float32, place=Place(gpu:0), stop_gradient=False,
       [0.30000001])
# In static mode, the forward b is right, but grad of a and v are not
>>> paddle.enable_static()
>>> exe = paddle.static.Executor()
>>>
>>> train_program = paddle.static.Program()
>>> startup_program = paddle.static.Program()
>>> with paddle.static.program_guard(train_program, startup_program):
...     a = paddle.full((2,3,4), 1.2, dtype='float32')
...     a.stop_gradient=False
...     v = paddle.ones((1,), dtype='float32')
...     v.stop_gradient=False
...     b = a * 2
...     b[0, :, 2] = b[0, :, 2] * v
...     l = b.mean()
...     paddle.static.append_backward(l)
...     res = exe.run(fetch_list=[b, a.grad_name, v.grad_name])
...     print('b.sum() =', res[0].sum())
...     print('a.grad.sum() = ', res[1].sum())
...     print('v.grad.sum() = ', res[2].sum())
...
b.sum() = 57.600002
a.grad.sum() =  1.75
v.grad.sum() =  0.0

The essence of this problem is that the method of directly rewriting variables in static graphs violates the static single assignment form (SSA).

What this PR changed

  • In Dynamic Mode:
    The behavior of __setitem__ is unchanged. Users can still write code like: x[0] = 10.

  • In Static Mode
    __setitem__ is disabled. Users should use x = paddle.static.setitem(x, 0, 10) to replace x[0] = 10.
    API paddle.static.setitem will provide the variable assignment with indexing in static mode, but unlike __setitem__, it will return a new variable instead of overwriting the original variable.

>>> paddle.enable_static()
>>> a = paddle.ones((2,3,4))
>>> a[0] = 10
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.7/dist-packages/paddle/fluid/framework.py", line 2303, in __setitem__
    "In static mode, the __setitem__ (looks like: x[indices] = values) should not be used. Please use x = paddle.static.setitem(x, indices, values)"
RuntimeError: In static mode, the __setitem__ (looks like: x[indices] = values) should not be used. Please use x = paddle.static.setitem(x, indices, values)
  • In Dynamic-to-Static (with @paddle.jit.to_static )
    As Fix setitem error in static mode #55282 , introducing a ParametersMap, if there are a new variable mapped to an old variable, the old one will be replaced by the new one.
    Users are not aware of this change in d2s, with just still using paddle.jit.to_static.

@paddle-bot
Copy link

paddle-bot bot commented May 10, 2023

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@paddle-bot
Copy link

paddle-bot bot commented May 10, 2023

❌ The PR is not created using PR's template. You can refer to this Demo.
Please use PR's template, it helps save our maintainers' time so that more developers get helped.

@paddle-ci-bot
Copy link

paddle-ci-bot bot commented May 18, 2023

Sorry to inform you that 788f8ac's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually.


def __setitem__(self, item, value):
if isinstance(self.obj, Variable):
return paddle.fluid.framework._setitem_impl_(self.obj, item, value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return paddle.fluid.framework._setitem_impl_(self.obj, item, value)
new_var = paddle.fluid.framework._setitem_impl_(self.obj, item, value)
# NOTE(dev): Update __dict__ will not modify the id(), but only move the
# pointed reference object to the new one.
self.obj.__dict__.update(new_var.__dict__)

@zoooo0820 zoooo0820 closed this Jul 11, 2023
@zoooo0820 zoooo0820 reopened this Jul 11, 2023
@zoooo0820 zoooo0820 marked this pull request as draft July 12, 2023 02:45
@zoooo0820 zoooo0820 marked this pull request as ready for review July 12, 2023 02:45
@zoooo0820 zoooo0820 changed the title [do not merge] test static setitem with dy2st disable __setitem__ in static mode & add API paddle.static.setitem with dy2st strategy Jul 13, 2023
Copy link
Contributor

@Aurelius84 Aurelius84 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jeff41404 jeff41404 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

@jeff41404 jeff41404 merged commit 7849d58 into PaddlePaddle:develop Jul 19, 2023
@zoooo0820 zoooo0820 deleted the fix_d2s branch July 19, 2023 02:32
cqulilujia pushed a commit to cqulilujia/Paddle that referenced this pull request Jul 24, 2023
…th dy2st strategy (PaddlePaddle#53682)

* add paddle.static.setitem

* add some help doc

* support setitem

* support machanism

* add more unittest

* remove usless code

* raise error in static setitem

* fix d2s UT

* remove static only for both-used code

* fix bool set_value in static, fix set_value_op UT

* fix unittests

* [May case some error]: remove inplace-version check

* add two test case for dy2st

* fix function in vision

* fix dy2st setitem support, refine UT case

* fix slice in static_mode

* add ParametersMap

* remove pop

* modify place

* [fix]: variable is also a tensor

* rewrite some ut & remove slicetransformer in dy2st

* solve error in static-mode

* fix ut

* return a result for set_array_write

* fix test_set_value_op_xpu

* code is different in dynamic / static mode

---------

Co-authored-by: Aurelius84 <zhangliujie@baidu.com>
Co-authored-by: NotHaozi <zhangmenghao@baidu.com>
wz1qqx pushed a commit to wz1qqx/Paddle that referenced this pull request Jul 31, 2023
…th dy2st strategy (PaddlePaddle#53682)

* add paddle.static.setitem

* add some help doc

* support setitem

* support machanism

* add more unittest

* remove usless code

* raise error in static setitem

* fix d2s UT

* remove static only for both-used code

* fix bool set_value in static, fix set_value_op UT

* fix unittests

* [May case some error]: remove inplace-version check

* add two test case for dy2st

* fix function in vision

* fix dy2st setitem support, refine UT case

* fix slice in static_mode

* add ParametersMap

* remove pop

* modify place

* [fix]: variable is also a tensor

* rewrite some ut & remove slicetransformer in dy2st

* solve error in static-mode

* fix ut

* return a result for set_array_write

* fix test_set_value_op_xpu

* code is different in dynamic / static mode

---------

Co-authored-by: Aurelius84 <zhangliujie@baidu.com>
Co-authored-by: NotHaozi <zhangmenghao@baidu.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
Development

Successfully merging this pull request may close these issues.

5 participants