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

[PY] refactoring - move devices-related functions from tvm.runtime.ndarray #11126

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

pfk-beta
Copy link
Contributor

Hello,

I'm not sure whether it needs RFC, but it was little messy in python runtime code (related to my answer, not main thread in this post https://discuss.tvm.apache.org/t/pre-rfc-more-meaningful-names-in-rpc-module-in-python/12618). I have moved device, cpu, cuda, gpu, opencl, cl, vulkan, metal, mtl, vpi, rocm, ext_dev functions from tvm.runtime.ndarray to tvm.runtime.devices and reimported them in tvm.runtime, so we can use following statement:

import tvm.runtime
runtime.cpu(0)

In previous version there was mix of following:

import tvm
tvm.cpu()
from tvm.runtime import ndarray as _nd
_nd.cuda(0)

@pfk-beta pfk-beta changed the title refactoring - move devices-related stuff from tvm.runtime.ndarray to … [PY] refactoring - move devices-related functions from tvm.runtime.ndarray Apr 26, 2022
@tqchen
Copy link
Member

tqchen commented Apr 26, 2022

Thanks @pfk-beta , the original rationale was that device is closely coupled with ndarray itself(as ndarray is the only places that device get allocated).

Re-exposing the devices under tvm.runtime namespace would indeed be a readability improvement. From UX's pov we intentionally expose tvm.cpu/tvm.cuda not for internal development but for users, so we would like to keep those exposures to avoid API breaking.

For the same reason(although weaker as there are less usages here), we might want to to keep nd.device usages when possible for one version. Personally i think putting devices definition as separate file or in ndarray do not have a big difference, although in this case it does bring a minor readability gain.

One simple approach is to re-expose nd.cpu etc in the runtime namespace, and use runtime.device in most of the places you mention. This would be the least surgical change while retains the readability part.

@pfk-beta
Copy link
Contributor Author

pfk-beta commented Apr 27, 2022

Re-exposing the devices under tvm.runtime namespace

You mean devices - functions, not module which I created, right? They were already exposed, I just standrized imports and usage of them. Even python api documentation list these functions in tvm.runtime (https://tvm.apache.org/docs/reference/api/python/runtime.html)

From UX's pov we intentionally expose tvm.cpu/tvm.cuda not for internal development but for users, so we would like to keep those exposures to avoid API breaking.

That is why, I didn't remove devices functions from top tvm module. I think we should put this informations in guideline for python developers, which want to contribute to TVM. Because I refactored mix of usage, and I don't want do this again :) Is there such guideline, or should we create one?

My proposition is:

  • we have devices.py with devices functions
  • they will be imported in ndarray, for backward compatibility (explain when we should use nd.<device>)
  • we still have devices availabe through import tvm and import tvm.runtime - runtime.<device> is used for most of the internal developers, tvm.<device> is used for tvm users.
  • we put these informations to python contribution guideline - is this place https://tvm.apache.org/docs/contribute/code_guide.html#python-code-styles suitable?

@tqchen
Copy link
Member

tqchen commented May 4, 2022

Thanks @pfk-beta . I agree this is a positive change towards readability. Please check the linter error(all new files needs ASF header). The overall convention of using tvm.runtime.device sounds good. Although I do not think we should prevent developers from using tvm.device itself as this is also what intended for the users

@pfk-beta
Copy link
Contributor Author

pfk-beta commented May 4, 2022

Although I do not think we should prevent developers from using tvm.device itself as this is also what intended for the users

To clarify, you mean using tvm.device.cuda or tvm.cuda?

@tqchen
Copy link
Member

tqchen commented May 4, 2022

sorry, i meant tvm.cuda

@areusch areusch added needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it and removed needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it labels Oct 19, 2022
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.

None yet

3 participants