Remove old umf leftovers #2214
Remove old umf leftovers #2214
Conversation
Pool tracking is enabled in UMF by default since: oneapi-src/unified-memory-framework#485
This label is not used in tests anymore.
github-actions
bot
added
ci/cd
|
@oneapi-src/unified-runtime-cuda-write, @oneapi-src/unified-runtime-hip-write, @GeorgeWeb - pls review |
| @@ -385,8 +385,6 @@ UR_APIEXPORT ur_result_t UR_APICALL urUSMPoolCreate( | |||
| ///< ::ur_usm_pool_limits_desc_t | |||
| ur_usm_pool_handle_t *Pool ///< [out] pointer to USM memory pool | |||
| ) { | |||
| // Without pool tracking we can't free pool allocations. | |||
| #ifdef UMF_ENABLE_POOL_TRACKING | |||
There was a problem hiding this comment.
Last time I checked, UMF_ENABLE_POOL_TRACKING was disabled in HIP (note the lack of CMake adding the compile definitions). Has something changed such that we're able to enable pool tracking by default in HIP?
There was a problem hiding this comment.
och, I've missed that, thanks for the review! On the other hand, the CI is green... so either we have some magic in testing or it works as expected... let me verify that further
There was a problem hiding this comment.
I forget what the specific problem was, but IIRC only some versions of HIP were having problems. See #1479 and #1689. I couldn't make much progress with it just ended up prioritising something else, but I see in my notes that I put it down to HIP 5.7.1 crashing in CI (at the time) whereas locally 6.0+ was okay.
I don't know what we're testing in CI these days, I'm afraid.
There was a problem hiding this comment.
Also, we don't run HIP E2E testing for DPC++, so I wonder if that's something we ought to verify too tho unlikely to happen since it looks like the Precommit CI for that target is not running in intel/llvm atm.
I am unaware what the UMF tracking problems with HIP were and I presume the passing CI, based on Fraser's previous observations, could be due to testing newer version of HIP these days.
There was a problem hiding this comment.
I've rebased that old PR of mine, just in case it is able to provide some more context/data about what's going on.
There was a problem hiding this comment.
Hey @lukaszstolarczuk
This PR #1689 removes some more USMAlloc tests from the match files and they seem to fail with enabled memory pools (via UMF_ENABLE_POOL_TRACKING). Maybe they should remain as known failures for now and we still have memory pools enabled by default but I am not sure of any implications.
I think I'll use yours and @frasercrmck's best judgement on this matter.
There was a problem hiding this comment.
I think, given that this PR is not NFC for HIP, we should see some changes to the match files. The PR description should also reflect this. If there are still failures in the USM tests with this PR (as I suspect there will be, given the problems I had on my PR) then we need to account for them somehow. This might be tricky given that CI isn't currently testing a supported GPU.
|
Should we be adding a counterpart intel/llvm PR that tests the latest state of the runtime and bumps the UR tag accordingly too? |
Unified Runtime -> intel/llvm Repo Move NoticeInformationThe source code of Unified Runtime has been moved to intel/llvm under the unified-runtime top-level directory, The code will be mirrored to oneapi-src/unified-runtime and the specification will continue to be hosted at oneapi-src.github.io/unified-runtime. The contribution guide has been updated with new instructions for contributing to Unified Runtime. PR MigrationAll open PRs including this one will be labelled auto-close and shall be automatically closed after 30 days. Should you wish to continue with your PR you will need to migrate it to intel/llvm. This is an automated comment. |
Unified Runtime -> intel/llvm Repo Move NoticeFollowing on from the previous notice, we have now enabled workflows to automatically label and close PRs because the Unified Runtime source code has moved to intel/llvm. This PR has now been marked with the Please review the previous notice for more information, including assistance with migrating your PR to intel/llvm. Should there be a reason for this PR to remain open, manually remove the This is an automated comment. |
umflabel is no longer used in UR's tests.