You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/10/20 03:46:38 UTC
[GitHub] [incubator-mxnet] Kh4L opened a new pull request #19386: [WIP] Move block.optimize_for backend_opts to kwargs
Kh4L opened a new pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386
## Description ##
`symbol.optimize_for` backend options are given as kwargs but the `block.optimize_for` ones are given as a separate `backend_opts` dict.
We end up with an API discrepancy, than can be confusing for the user. Here's an exemple:
#### Symbol
```
sym.optimize_for(backend, args, reqArgs=True, dedup_subgraph=True)```
```
#### Block (Gluon)
```
blk.optimize_for(x, backend="addInputPass", clear=False, backend_opts={'dedup_subgraph':True})
```
This limitation was due to preserve the kwargs from hybridize.
However this kwargs is virtually only used to forward the `static_alloc` and `static_shape` arguments.
## Change
This PR factors out `static_alloc` and `static_shape` and changes the `kwargs`.
### Example after the change:
```
blk.optimize_for(x, backend="addInputPass", clear=False, backend_opts={'dedup_subgraph':True})
```
will be now called as
```
blk.optimize_for(x, backend="addInputPass", clear=False, dedup_subgraph=True)
```
Note: if needed in the future, we can add an `extra_hybridize_kwargs`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523366880
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1149,30 +1156,43 @@ def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **
The name of backend, as registered in `SubgraphBackendRegistry`, default None
backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
- static_alloc : bool, default False
+ clear : bool, default True
+ Clears any previous optimizations
+ static_alloc : optional bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
- static_shape : bool, default False
+ static_shape : optional bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ inline_limit : optional int, default 2
+ Maximum number of operators that can be inlined.
+ forward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ backward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
"""
Review comment:
It's added in `optimize_for`, but there is no more kwargs in hybridize, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r517067994
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1059,7 +1059,7 @@ def _call_cached_op(self, *args):
out = [out]
return _regroup(out, self._out_format)
- def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **kwargs):
+ def optimize_for(self, x, *args, backend=None, clear=True, static_alloc=False, static_shape=False, **kwargs):
Review comment:
I like this, can we do the same for hybridize and eliminate `backend_opts` altogether?
Change:
```
def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **kwargs):
```
To:
```
def hybridize(self, active=True, backend=None, clear=True, static_alloc=False, static_shape=False, **kwargs):
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#issuecomment-722691118
I guess we need to add `inline_limit` in Python too and pass it down:
https://github.com/apache/incubator-mxnet/blob/87b66a9e1b83d66f3c0f893ca629e86b43966994/src/imperative/cached_op.h#L331-L337
But I dont know if we need the rest
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523366648
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1059,7 +1059,7 @@ def _call_cached_op(self, *args):
out = [out]
return _regroup(out, self._out_format)
- def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **kwargs):
+ def optimize_for(self, x, *args, backend=None, clear=False, static_alloc=False, static_shape=False, **kwargs):
Review comment:
I thought that this would clutter the args.
inline_limit=2, forward_bulk_size=None, backward_bulk_size=None being rarely use, I think it's fine to let the user call hybridize separately if he needs them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523236944
##########
File path: example/extensions/lib_subgraph/README.md
##########
@@ -107,15 +107,15 @@ The `optimize_for` API takes at least 1 argument, `backend` which is a string th
For the Gluon API, `hybridize` can be called on HybridBlocks to partition the internal CachedOp Symbol.
```python
-block.hybridize(backend=None, backend_opts=None, clear=True, **kwargs)
+block.hybridize(backend=None, clear=True, **kwargs)
Review comment:
Most of hybridize calls assume that `clear=True`, so I thought that it would be safer to keep it as it is.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523369938
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1149,30 +1156,43 @@ def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **
The name of backend, as registered in `SubgraphBackendRegistry`, default None
backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
- static_alloc : bool, default False
+ clear : bool, default True
+ Clears any previous optimizations
+ static_alloc : optional bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
- static_shape : bool, default False
+ static_shape : optional bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ inline_limit : optional int, default 2
+ Maximum number of operators that can be inlined.
+ forward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ backward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
"""
Review comment:
we still need to take backend args in hybridize. so after making all the cachedOp kwargs actual args in the function, kwargs will become just backend options
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky merged pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky merged pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mseth10 commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
mseth10 commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r521709245
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1087,19 +1087,23 @@ def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **
other inputs to model
backend : str
The name of backend, as registered in `SubgraphBackendRegistry`, default None
- backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
- Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
+ clear : bool, default False
+ Clears any previous optimizations
static_alloc : bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
static_shape : bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ **kwargs: The backend options, optional
+ Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
"""
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
- # do hybrize API call
- self.hybridize(True, backend, backend_opts, clear, **kwargs)
+ if clear or not self._active:
Review comment:
why do we need this condition now?
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1087,19 +1087,23 @@ def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **
other inputs to model
backend : str
The name of backend, as registered in `SubgraphBackendRegistry`, default None
- backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
- Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
+ clear : bool, default False
+ Clears any previous optimizations
static_alloc : bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
static_shape : bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ **kwargs: The backend options, optional
+ Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
"""
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
- # do hybrize API call
- self.hybridize(True, backend, backend_opts, clear, **kwargs)
+ if clear or not self._active:
+ # do hybrize API call
Review comment:
nit: hybridize
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1087,19 +1087,23 @@ def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **
other inputs to model
backend : str
The name of backend, as registered in `SubgraphBackendRegistry`, default None
- backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
- Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
+ clear : bool, default False
+ Clears any previous optimizations
static_alloc : bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
static_shape : bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ **kwargs: The backend options, optional
+ Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
"""
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
- # do hybrize API call
- self.hybridize(True, backend, backend_opts, clear, **kwargs)
+ if clear or not self._active:
+ # do hybrize API call
+ self.hybridize(True, backend, clear, static_alloc=static_alloc, static_shape=static_shape)
Review comment:
we might have to rebase this PR, as we recently added another parameter `partition_if_dynamic` to both hybridize and optimize_for
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1137,7 +1141,11 @@ def register_child(self, block, name=None):
super(HybridBlock, self).register_child(block, name)
self._clear_cached_op()
- def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **kwargs):
+ def hybridize(self, active=True, backend=None, clear=True,
Review comment:
we still need kwargs for hybridize, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r521765372
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1137,7 +1141,11 @@ def register_child(self, block, name=None):
super(HybridBlock, self).register_child(block, name)
self._clear_cached_op()
- def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **kwargs):
+ def hybridize(self, active=True, backend=None, clear=True,
Review comment:
this PR formalizes the args that get passed to CachedOp as parameters to hybridize. Theres no need for kwargs anymore.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523375529
##########
File path: example/extensions/lib_subgraph/README.md
##########
@@ -107,15 +107,15 @@ The `optimize_for` API takes at least 1 argument, `backend` which is a string th
For the Gluon API, `hybridize` can be called on HybridBlocks to partition the internal CachedOp Symbol.
```python
-block.hybridize(backend=None, backend_opts=None, clear=True, **kwargs)
+block.hybridize(backend=None, clear=True, **kwargs)
```
-The `hybridize` function prepares the HybridBlock to be converted into a backend symbol. The `backend` argument is a string that identifies which backend that will partition the model. The `backend_opts` are other user-specified options (as a Python dictionary of strings mapped to strings) that will be passed to the backend partitioning APIs. The `clear` argument defaults to `True` and clears any previous optimizations done on the block. If you want to chain optimizations together, set `clear` to `False`. The actual partitioning takes place during the forward pass. If you want to use `hybridize` to chain multiple optimizations, be sure to execute a forward pass after each call to `hybridize`.
+The `hybridize` function prepares the HybridBlock to be converted into a backend symbol. The `backend` argument is a string that identifies which backend that will partition the model. `**kwargs` are other user-specified options (as a Python dictionary of strings mapped to strings) that will be passed to the backend partitioning APIs. The `clear` argument defaults to `False`, so it will chain optimizations together. If you want to clear clear any previous optimizations done on the block, set `clear` to `True`. The actual partitioning takes place during the forward pass. If you want to use `hybridize` to chain multiple optimizations, be sure to execute a forward pass after each call to `hybridize`.
If you just want to partition the HybridBlock but not run a complete forward pass, you can use the `optimize_for` API that combines the work done in the `hybridize` API with part of the work done in the forward pass.
```python
-block.optimize_for(x, backend=None, backend_opts=None, clear=True, **kwargs)
+block.optimize_for(x, backend=None, clear=True, **kwargs)
Review comment:
clear = False
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523231785
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1087,19 +1087,23 @@ def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **
other inputs to model
backend : str
The name of backend, as registered in `SubgraphBackendRegistry`, default None
- backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
- Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
+ clear : bool, default False
+ Clears any previous optimizations
static_alloc : bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
static_shape : bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ **kwargs: The backend options, optional
+ Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
"""
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
- # do hybrize API call
- self.hybridize(True, backend, backend_opts, clear, **kwargs)
+ if clear or not self._active:
+ # do hybrize API call
+ self.hybridize(True, backend, clear, static_alloc=static_alloc, static_shape=static_shape)
Review comment:
As discussed offline with @samskalicky , completely separating hybridize+optimize_for is an API breaking change, so we will keep it for 2.0
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19386: [WIP] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#issuecomment-712569876
Hey @Kh4L , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:
- To trigger all jobs: @mxnet-bot run ci [all]
- To trigger specific jobs: @mxnet-bot run ci [job1, job2]
***
**CI supported jobs**: [edge, unix-gpu, unix-cpu, windows-gpu, miscellaneous, sanity, centos-cpu, centos-gpu, clang, website, windows-cpu]
***
_Note_:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mseth10 commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
mseth10 commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r521767451
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1137,7 +1141,11 @@ def register_child(self, block, name=None):
super(HybridBlock, self).register_child(block, name)
self._clear_cached_op()
- def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **kwargs):
+ def hybridize(self, active=True, backend=None, clear=True,
Review comment:
How would users pass backend_opts with hybridize now?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523376398
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1087,19 +1093,30 @@ def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **
other inputs to model
backend : str
The name of backend, as registered in `SubgraphBackendRegistry`, default None
- backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
- Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
+ clear : bool, default False
+ Clears any previous optimizations
static_alloc : bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
static_shape : bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ inline_limit : optional int, default 2
+ Maximum number of operators that can be inlined.
+ forward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ backward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ **kwargs: The backend options, optional
+ Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
"""
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
- # do hybrize API call
- self.hybridize(True, backend, backend_opts, clear, **kwargs)
+ if clear or not self._active:
+ self.hybridize(True, backend, clear, static_alloc=static_alloc, static_shape=static_shape,
+ inline_limit=inline_limit, forward_bulk_size=forward_bulk_size,
+ backward_bulk_size=backward_bulk_size)
Review comment:
not really, I guess it just felt safer 😄
but you are right, it's too clunky
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523240675
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1149,30 +1156,43 @@ def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **
The name of backend, as registered in `SubgraphBackendRegistry`, default None
backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
- static_alloc : bool, default False
+ clear : bool, default True
Review comment:
are we changing the clear default for optimize_for, but leaving the default to True for hybridize? That seems to make sense, existing behavior for hyridize was to clear on every call so that makes sense. Just checking
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523241764
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1149,30 +1156,43 @@ def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **
The name of backend, as registered in `SubgraphBackendRegistry`, default None
backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
- static_alloc : bool, default False
+ clear : bool, default True
+ Clears any previous optimizations
+ static_alloc : optional bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
- static_shape : bool, default False
+ static_shape : optional bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ inline_limit : optional int, default 2
+ Maximum number of operators that can be inlined.
+ forward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ backward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
"""
Review comment:
should we add a comment that kwargs goes to backend?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523237297
##########
File path: example/extensions/lib_subgraph/README.md
##########
@@ -107,15 +107,15 @@ The `optimize_for` API takes at least 1 argument, `backend` which is a string th
For the Gluon API, `hybridize` can be called on HybridBlocks to partition the internal CachedOp Symbol.
```python
-block.hybridize(backend=None, backend_opts=None, clear=True, **kwargs)
+block.hybridize(backend=None, clear=True, **kwargs)
Review comment:
It's slightly different that the `optimize_for` case, the user might call several `optimize_for`, but will usually only call hybridize once at the beginning.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mseth10 commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
mseth10 commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r521763901
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1137,7 +1141,11 @@ def register_child(self, block, name=None):
super(HybridBlock, self).register_child(block, name)
self._clear_cached_op()
- def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **kwargs):
+ def hybridize(self, active=True, backend=None, clear=True,
Review comment:
why remove kwargs from hybridize?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r518412778
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1156,23 +1157,23 @@ def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ **kwargs : dict
+ Optional backend options when hybridized is called with an optimization backend.
"""
self._backend = backend
- if backend_opts is not None:
- assert isinstance(backend_opts, dict), \
- "HybridBlock hybridize requires backend_opts to be a dictionary."
- self._backend_opts = backend_opts
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
Review comment:
can we get rid of `_backend_opts` now?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523366648
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1059,7 +1059,7 @@ def _call_cached_op(self, *args):
out = [out]
return _regroup(out, self._out_format)
- def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **kwargs):
+ def optimize_for(self, x, *args, backend=None, clear=False, static_alloc=False, static_shape=False, **kwargs):
Review comment:
I thought that this would clutter the args.
inline_limit=2, forward_bulk_size=None, backward_bulk_size=None being rarely use, I think it's fine to let the user call hybridize separately if he needs them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r521825721
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1137,7 +1141,11 @@ def register_child(self, block, name=None):
super(HybridBlock, self).register_child(block, name)
self._clear_cached_op()
- def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **kwargs):
+ def hybridize(self, active=True, backend=None, clear=True,
Review comment:
They have to pass them through `optimize_for`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523239152
##########
File path: example/extensions/lib_subgraph/README.md
##########
@@ -107,15 +107,15 @@ The `optimize_for` API takes at least 1 argument, `backend` which is a string th
For the Gluon API, `hybridize` can be called on HybridBlocks to partition the internal CachedOp Symbol.
```python
-block.hybridize(backend=None, backend_opts=None, clear=True, **kwargs)
+block.hybridize(backend=None, clear=True, **kwargs)
```
-The `hybridize` function prepares the HybridBlock to be converted into a backend symbol. The `backend` argument is a string that identifies which backend that will partition the model. The `backend_opts` are other user-specified options (as a Python dictionary of strings mapped to strings) that will be passed to the backend partitioning APIs. The `clear` argument defaults to `True` and clears any previous optimizations done on the block. If you want to chain optimizations together, set `clear` to `False`. The actual partitioning takes place during the forward pass. If you want to use `hybridize` to chain multiple optimizations, be sure to execute a forward pass after each call to `hybridize`.
+The `hybridize` function prepares the HybridBlock to be converted into a backend symbol. The `backend` argument is a string that identifies which backend that will partition the model. `**kwargs` are other user-specified options (as a Python dictionary of strings mapped to strings) that will be passed to the backend partitioning APIs. The `clear` argument defaults to `True` and clears any previous optimizations done on the block. If you want to chain optimizations together, set `clear` to `False`. The actual partitioning takes place during the forward pass. If you want to use `hybridize` to chain multiple optimizations, be sure to execute a forward pass after each call to `hybridize`.
Review comment:
change " The `clear` argument defaults to `True` "
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523366755
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1149,30 +1156,43 @@ def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **
The name of backend, as registered in `SubgraphBackendRegistry`, default None
backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
- static_alloc : bool, default False
+ clear : bool, default True
Review comment:
That's correct!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r521775055
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1087,19 +1087,23 @@ def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **
other inputs to model
backend : str
The name of backend, as registered in `SubgraphBackendRegistry`, default None
- backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
- Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
+ clear : bool, default False
+ Clears any previous optimizations
static_alloc : bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
static_shape : bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ **kwargs: The backend options, optional
+ Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
"""
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
- # do hybrize API call
- self.hybridize(True, backend, backend_opts, clear, **kwargs)
+ if clear or not self._active:
+ # do hybrize API call
+ self.hybridize(True, backend, clear, static_alloc=static_alloc, static_shape=static_shape)
Review comment:
@Kh4L why dont we just separate out the hybridize & optimize_for flows. We'll leave hybridize as the way to set CachedOp flags. Optimize_for will only need to set _active=True, thats the only line that would be duplicated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r517067994
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1059,7 +1059,7 @@ def _call_cached_op(self, *args):
out = [out]
return _regroup(out, self._out_format)
- def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **kwargs):
+ def optimize_for(self, x, *args, backend=None, clear=True, static_alloc=False, static_shape=False, **kwargs):
Review comment:
I like this, can we do the same for hybridize and eliminate backend_opts altogether?
Change:
```
def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **kwargs):
```
To:
```
def hybridize(self, active=True, backend=None, clear=True, static_alloc=False, static_shape=False, **kwargs):
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523239782
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1059,7 +1059,7 @@ def _call_cached_op(self, *args):
out = [out]
return _regroup(out, self._out_format)
- def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **kwargs):
+ def optimize_for(self, x, *args, backend=None, clear=False, static_alloc=False, static_shape=False, **kwargs):
Review comment:
dont we need to add the other hybridize args like: inline_limit=2, forward_bulk_size=None, backward_bulk_size=None
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523241228
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1137,7 +1141,11 @@ def register_child(self, block, name=None):
super(HybridBlock, self).register_child(block, name)
self._clear_cached_op()
- def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **kwargs):
+ def hybridize(self, active=True, backend=None, clear=True,
Review comment:
Given the latest decision, we're going to make all CachedOp options explicit and all kwargs will go to backend/optimize_for
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r518323931
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1059,7 +1059,7 @@ def _call_cached_op(self, *args):
out = [out]
return _regroup(out, self._out_format)
- def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **kwargs):
+ def optimize_for(self, x, *args, backend=None, clear=True, static_alloc=False, static_shape=False, **kwargs):
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#issuecomment-719091783
Jenkins CI successfully triggered : [edge, unix-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r521765806
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1087,19 +1087,23 @@ def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **
other inputs to model
backend : str
The name of backend, as registered in `SubgraphBackendRegistry`, default None
- backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
- Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
+ clear : bool, default False
+ Clears any previous optimizations
static_alloc : bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
static_shape : bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ **kwargs: The backend options, optional
+ Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
"""
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
- # do hybrize API call
- self.hybridize(True, backend, backend_opts, clear, **kwargs)
+ if clear or not self._active:
+ # do hybrize API call
+ self.hybridize(True, backend, clear, static_alloc=static_alloc, static_shape=static_shape)
Review comment:
#18690 for reference is on master branch. this PR is on v1.x so we're ok for now. But we'll need to handle this when we port this to master
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523382226
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1087,19 +1093,29 @@ def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **
other inputs to model
backend : str
The name of backend, as registered in `SubgraphBackendRegistry`, default None
- backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
- Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
+ clear : bool, default False
+ Clears any previous optimizations
static_alloc : bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
static_shape : bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ inline_limit : optional int, default 2
+ Maximum number of operators that can be inlined.
+ forward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ backward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ **kwargs: The backend options, optional
+ Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
"""
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
- # do hybrize API call
- self.hybridize(True, backend, backend_opts, clear, **kwargs)
+ if clear or not self._active:
+ self.hybridize(True, backend, clear, static_alloc, static_shape,
+ inline_limit, forward_bulk_size, backward_bulk_size, kwargs)
Review comment:
Good cqtch
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1087,19 +1093,29 @@ def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **
other inputs to model
backend : str
The name of backend, as registered in `SubgraphBackendRegistry`, default None
- backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
- Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
+ clear : bool, default False
+ Clears any previous optimizations
static_alloc : bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
static_shape : bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ inline_limit : optional int, default 2
+ Maximum number of operators that can be inlined.
+ forward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ backward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ **kwargs: The backend options, optional
+ Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
"""
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
- # do hybrize API call
- self.hybridize(True, backend, backend_opts, clear, **kwargs)
+ if clear or not self._active:
+ self.hybridize(True, backend, clear, static_alloc, static_shape,
+ inline_limit, forward_bulk_size, backward_bulk_size, kwargs)
Review comment:
Good catch
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523376185
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1149,30 +1156,43 @@ def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **
The name of backend, as registered in `SubgraphBackendRegistry`, default None
backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
- static_alloc : bool, default False
+ clear : bool, default True
+ Clears any previous optimizations
+ static_alloc : optional bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
- static_shape : bool, default False
+ static_shape : optional bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ inline_limit : optional int, default 2
+ Maximum number of operators that can be inlined.
+ forward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ backward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
"""
Review comment:
self._backend_opts = kwargs here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
Kh4L commented on pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#issuecomment-722810985
> I guess we need to add `inline_limit` in Python too and pass it down:
> https://github.com/apache/incubator-mxnet/blob/87b66a9e1b83d66f3c0f893ca629e86b43966994/src/imperative/cached_op.h#L331-L337
>
>
> But I dont know if we need the rest. Initial look seems that we need the bulk_size ones, but thats it.
Good catch, I added those 3 args!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r521775055
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1087,19 +1087,23 @@ def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **
other inputs to model
backend : str
The name of backend, as registered in `SubgraphBackendRegistry`, default None
- backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
- Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
+ clear : bool, default False
+ Clears any previous optimizations
static_alloc : bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
static_shape : bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ **kwargs: The backend options, optional
+ Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
"""
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
- # do hybrize API call
- self.hybridize(True, backend, backend_opts, clear, **kwargs)
+ if clear or not self._active:
+ # do hybrize API call
+ self.hybridize(True, backend, clear, static_alloc=static_alloc, static_shape=static_shape)
Review comment:
@Kh4L why dont we just separate out the hybridize & optimize_for flows. We'll leave hybridize as the way to set CachedOp flags. Optimize_for will only need to set _active=True, thats the only line that would be duplicated. setting backend would only be done in optimize_for. This will achieve the goal you set out to: removing backend_opts and enable specifying those options via kwargs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
Kh4L commented on pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#issuecomment-719091741
@mxnet-bot run ci [edge, unix-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523386678
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1147,32 +1168,47 @@ def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **
Whether to turn hybrid on or off.
backend : str
The name of backend, as registered in `SubgraphBackendRegistry`, default None
- backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
- Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
- static_alloc : bool, default False
+ clear : bool, default True
+ Clears any previous optimizations
+ static_alloc : optional bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
- static_shape : bool, default False
+ static_shape : optional bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ inline_limit : optional int, default 2
+ Maximum number of operators that can be inlined.
+ forward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ backward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ **kwargs: optional
+ Backend options.
"""
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
self._backend = backend
- if backend_opts is not None:
- assert isinstance(backend_opts, dict), \
- "HybridBlock hybridize requires backend_opts to be a dictionary."
- self._backend_opts = backend_opts
self._active = active
- self._flags = list(kwargs.items())
+ self._flags = [("static_alloc", static_alloc), ("static_shape", static_shape),
+ ("inline_limit", inline_limit)]
+ if forward_bulk_size is not None:
+ self._flags.append(("forward_bulk_size", forward_bulk_size))
+ if backward_bulk_size is not None:
+ self._flags.append(("backward_bulk_size", backward_bulk_size))
if clear:
self._clear_cached_op()
if active and self._forward_hooks or self._forward_pre_hooks:
warnings.warn('"{block}" is being hybridized while still having forward hook/pre-hook. '
'If "{block}" is a child of HybridBlock, the hooks will not take effect.'
.format(block=self))
- super(HybridBlock, self).hybridize(active, **kwargs)
+ super(HybridBlock, self).hybridize(active,
Review comment:
no we can leave this as-is for the Block class since it doesnt really care about what the kwargs are anyway, its just going to pass them all recursively to its children. It would also be a 2nd place were the args would be duplicated for no benefit, creating a maintenance issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r521766129
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1087,19 +1087,23 @@ def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **
other inputs to model
backend : str
The name of backend, as registered in `SubgraphBackendRegistry`, default None
- backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
- Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
+ clear : bool, default False
+ Clears any previous optimizations
static_alloc : bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
static_shape : bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ **kwargs: The backend options, optional
+ Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
"""
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
- # do hybrize API call
- self.hybridize(True, backend, backend_opts, clear, **kwargs)
+ if clear or not self._active:
Review comment:
Now, we only call hybridize from optimize_for once the first time (2nd time _active is set) or if the user sets the clear flag
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mseth10 commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
mseth10 commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523384918
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1147,32 +1168,47 @@ def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **
Whether to turn hybrid on or off.
backend : str
The name of backend, as registered in `SubgraphBackendRegistry`, default None
- backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
- Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
- static_alloc : bool, default False
+ clear : bool, default True
+ Clears any previous optimizations
+ static_alloc : optional bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
- static_shape : bool, default False
+ static_shape : optional bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ inline_limit : optional int, default 2
+ Maximum number of operators that can be inlined.
+ forward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ backward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ **kwargs: optional
+ Backend options.
"""
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
self._backend = backend
- if backend_opts is not None:
- assert isinstance(backend_opts, dict), \
- "HybridBlock hybridize requires backend_opts to be a dictionary."
- self._backend_opts = backend_opts
self._active = active
- self._flags = list(kwargs.items())
+ self._flags = [("static_alloc", static_alloc), ("static_shape", static_shape),
+ ("inline_limit", inline_limit)]
+ if forward_bulk_size is not None:
+ self._flags.append(("forward_bulk_size", forward_bulk_size))
+ if backward_bulk_size is not None:
+ self._flags.append(("backward_bulk_size", backward_bulk_size))
if clear:
self._clear_cached_op()
if active and self._forward_hooks or self._forward_pre_hooks:
warnings.warn('"{block}" is being hybridized while still having forward hook/pre-hook. '
'If "{block}" is a child of HybridBlock, the hooks will not take effect.'
.format(block=self))
- super(HybridBlock, self).hybridize(active, **kwargs)
+ super(HybridBlock, self).hybridize(active,
Review comment:
Should we explicitly specify args for Block.hybridize as well now that we have done it here?
https://github.com/apache/incubator-mxnet/blob/09d0cc8418cddefddf3f03aeeb1cbeb1fd4cbafa/python/mxnet/gluon/block.py#L658-L662
##########
File path: example/extensions/lib_subgraph/README.md
##########
@@ -107,15 +107,15 @@ The `optimize_for` API takes at least 1 argument, `backend` which is a string th
For the Gluon API, `hybridize` can be called on HybridBlocks to partition the internal CachedOp Symbol.
```python
-block.hybridize(backend=None, backend_opts=None, clear=True, **kwargs)
+block.hybridize(backend=None, clear=True, **kwargs)
```
-The `hybridize` function prepares the HybridBlock to be converted into a backend symbol. The `backend` argument is a string that identifies which backend that will partition the model. The `backend_opts` are other user-specified options (as a Python dictionary of strings mapped to strings) that will be passed to the backend partitioning APIs. The `clear` argument defaults to `True` and clears any previous optimizations done on the block. If you want to chain optimizations together, set `clear` to `False`. The actual partitioning takes place during the forward pass. If you want to use `hybridize` to chain multiple optimizations, be sure to execute a forward pass after each call to `hybridize`.
+The `hybridize` function prepares the HybridBlock to be converted into a backend symbol. The `backend` argument is a string that identifies which backend that will partition the model. `**kwargs` are other user-specified options (as a Python dictionary of strings mapped to strings) that will be passed to the backend partitioning APIs. The `clear` argument defaults to `False`, so it will chain optimizations together. If you want to clear clear any previous optimizations done on the block, set `clear` to `True`. The actual partitioning takes place during the forward pass. If you want to use `hybridize` to chain multiple optimizations, be sure to execute a forward pass after each call to `hybridize`.
Review comment:
nit: clear clear -> clear
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523241452
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1149,30 +1156,43 @@ def hybridize(self, active=True, backend=None, backend_opts=None, clear=True, **
The name of backend, as registered in `SubgraphBackendRegistry`, default None
backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
Review comment:
we should remove this arg/comment
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky edited a comment on pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky edited a comment on pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#issuecomment-722691118
I guess we need to add `inline_limit` in Python too and pass it down:
https://github.com/apache/incubator-mxnet/blob/87b66a9e1b83d66f3c0f893ca629e86b43966994/src/imperative/cached_op.h#L331-L337
But I dont know if we need the rest. Initial look seems that we need the bulk_size ones, but thats it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523381931
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1087,19 +1093,29 @@ def optimize_for(self, x, *args, backend=None, backend_opts=None, clear=True, **
other inputs to model
backend : str
The name of backend, as registered in `SubgraphBackendRegistry`, default None
- backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
- Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
- clear : clears any previous optimizations
+ clear : bool, default False
+ Clears any previous optimizations
static_alloc : bool, default False
Statically allocate memory to improve speed. Memory usage may increase.
static_shape : bool, default False
Optimize for invariant input shapes between iterations. Must also
set static_alloc to True. Change of input shapes is still allowed
but slower.
+ inline_limit : optional int, default 2
+ Maximum number of operators that can be inlined.
+ forward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ backward_bulk_size : optional int, default None
+ Segment size of bulk execution during forward pass.
+ **kwargs: The backend options, optional
+ Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
"""
+ if len(kwargs) > 0:
+ self._backend_opts = kwargs
- # do hybrize API call
- self.hybridize(True, backend, backend_opts, clear, **kwargs)
+ if clear or not self._active:
+ self.hybridize(True, backend, clear, static_alloc, static_shape,
+ inline_limit, forward_bulk_size, backward_bulk_size, kwargs)
Review comment:
we dont need to pass kwargs here since we just set them above
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523375621
##########
File path: example/extensions/lib_subgraph/test_subgraph.py
##########
@@ -103,14 +103,14 @@ def test(backend):
sym_block2 = nn.SymbolBlock(sym, inputs)
sym_block2.initialize()
sym_block2.optimize_for(mx.nd.ones((3,2)), mx.nd.ones((3,2)), backend=backend,
- backend_opts={'dedup_subgraph':True})
+ dedup_subgraph=True)
sym_block2.export('partitioned')
# Test with additional input to subgraph op
print('-------------------------------')
print('Testing %s Gluon Hybridize partitioning with extra input' % backend)
sym_block2.optimize_for(mx.nd.ones((3,2)), mx.nd.ones((3,2)), backend="addInputPass",
- clear=False, backend_opts={'dedup_subgraph':True})
+ clear=False, dedup_subgraph=True)
Review comment:
should we remove the clear=False now that its the default?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #19386: [1.x] Move block.optimize_for backend_opts to kwargs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19386:
URL: https://github.com/apache/incubator-mxnet/pull/19386#discussion_r523235729
##########
File path: example/extensions/lib_subgraph/README.md
##########
@@ -107,15 +107,15 @@ The `optimize_for` API takes at least 1 argument, `backend` which is a string th
For the Gluon API, `hybridize` can be called on HybridBlocks to partition the internal CachedOp Symbol.
```python
-block.hybridize(backend=None, backend_opts=None, clear=True, **kwargs)
+block.hybridize(backend=None, clear=True, **kwargs)
Review comment:
should we change this now that the default for clear is False?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org