Skip to content

enhance copy in metatensor#4506

Merged
wyli merged 3 commits intoProject-MONAI:feature/MetaTensorfrom
wyli:perf-tests
Jun 15, 2022
Merged

enhance copy in metatensor#4506
wyli merged 3 commits intoProject-MONAI:feature/MetaTensorfrom
wyli:perf-tests

Conversation

@wyli
Copy link
Copy Markdown
Contributor

@wyli wyli commented Jun 15, 2022

Signed-off-by: Wenqi Li [email protected]

addresses #4462 (comment)

Description

  • the _copy_meta is currently making outputs = model(image) slow because of the meta attributes deepcop, this PR updates it to use a shallow copy, now the fast training pipeline can achieve the expected speed
  • the PR makes get_default_meta static to have some minor speed up.

Status

Ready/Work in progress/Hold

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@wyli wyli requested review from Nic-Ma and rijobro June 15, 2022 06:13
@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Jun 15, 2022

Hi @rijobro, this PR is mainly to set deepcopy=False here:

deep_copy = id(self) != id_in
, so that we speed up the network training. I found that the deepcopy is the root cause of the slowness during network training (issue described here #4462 (comment)). Do you have any other suggestions/comments before I spend some time on this PR?

@rijobro
Copy link
Copy Markdown
Contributor

rijobro commented Jun 15, 2022

Hm, although disabling deep copying will make things faster, we obviously need to be careful of accidentally modifying an image's meta data when modifying another image that is a shallow copy. I suppose id(self) != id_in is limited in that with a = b, both a and b have different ID's, but they are shallow copies of one another and so here a shallow copy would suffice.

I'm not sure what the easy answer here is, we need a cleverer mechanism for figuring out when to copy and when not.

@rijobro
Copy link
Copy Markdown
Contributor

rijobro commented Jun 15, 2022

I don't think this is necessarily a good idea, but we could have a context manager:

with MetaTensor.deep_copy_meta(False):
    out = model(image

@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Jun 15, 2022

/integration-test

I don't think this is necessarily a good idea, but we could have a context manager:

with MetaTensor.deep_copy_meta(False):
    out = model(image

I'm trying to set shallow copy when is_batch is True, essentially when it's a batch, it's within a network forward and the meta info are not updated within it.

@wyli wyli marked this pull request as ready for review June 15, 2022 12:52
@rijobro
Copy link
Copy Markdown
Contributor

rijobro commented Jun 15, 2022

I suppose when it's in a batch we're unlikely to be modifying meta data. Seems like a bit of an indirect correlation, though.

@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Jun 15, 2022

the existing unit/integration tests work fine and the changes are non-breaking, I'll merge this for now but please feel free to create better solutions.

@wyli wyli merged commit df51ef4 into Project-MONAI:feature/MetaTensor Jun 15, 2022
@wyli wyli deleted the perf-tests branch June 15, 2022 13:56
@wyli wyli mentioned this pull request Jun 15, 2022
12 tasks
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.

2 participants