@Krovatkin I assume this line is not important. (i also assume you wrote it, maybe @desertfire did?)

yes you can remove it. When core folks fixed convolution to flow through one op, @desertfire removed the graph checks since convolutions become just another op

As we land these bindings, i'm wondering if we should try to simplify by not having helper functions like 'StepMarker'.

Taking the full view of the mark step API flow, it looks like

1 torch._lazy.mark_step (python code in torch/_lazy/init.py)
2 -> torch._C._lazy._mark_step (binding code in lazy/python/init.cpp)
3 -> StepMarker (local helper function in lazy/python/init.cpp)
4 -> finally calls torch::lazy::blah APIs

(2) can always be squished into (3) but that can make gdb debugging more annoying. But I also wonder if we can simplify to only have 3 layers, and generally flush stuff that would be in (2) either up into python in (1) or down into the core APIs (4).

Thoughts? @Krovatkin @alanwaketan

I think this is just Google's coding style to prefer wrapping a pile of code into a function just for readability not reusability.

I'm fine either way.

(2) can always be squished into (3) but that can make gdb debugging more annoying.

If users don't really need a python binding for 2), I'd suggest we go for it.

@JackCaoG here's a great example to discuss. How do you envision the mark_step API working when torch-xla is part of torch::lazy? Do you envision your users would ultimately import torch._lazy.mark_step, once you get past the stages of migration?

note: I did not intend to stop supporting _run_step_closures() but I might consider landing without it until we actually add some test coverage / usage scenarios of it.

Generally, my approach would be to only land the minimum bindings/apis we know we need and then keep adding them, rather than just land a bunch up front..

In my vision torch._lazy.mark_step and xm.mark_step will do the same thing after we finish the Lazytensor Class migration. They will all trigger a Synctensor. We will slowly redirect user to use lazy version of apis and update our own tutorial.

I see one small difference here. lazy:0 or in our case xla:0 is not always the default device. In our TPU training case xla:0 is a CPU and xla:1 is a TPU. I think we discussed this before and you guys don't want to have a GetDefaultDevice or GetCurrentDevice API. Is this still true?

I remember the decision is to add the GetDefaultDevice API? It's just that we never find time to do that. Maybe we should add a comment above.

I think we actually do have a 'GetDefaultDevice' API for the backend, but we assume it is static after initialization time. Maybe what I didn't want to do was have 'set/get current device' and have some state changing.

I was also thinking about how to improve our configuration flags and backend init APIs. If we provide a good way to configure the backend configuration (python binding and then ENV override for each flag) could you use one of those flags to set default device type? Or do you need something more than that?

This is the dummy stub we have in the staging branch:

In my vision torch._lazy.mark_step and xm.mark_step will do the same thing after we finish the Lazytensor Class migration. They will all trigger a Synctensor. We will slowly redirect user to use lazy version of apis and update our own tutorial.

I see one small difference here. lazy:0 or in our case xla:0 is not always the default device. In our TPU training case xla:0 is a CPU and xla:1 is a TPU. I think we discussed this before and you guys don't want to have a GetDefaultDevice or GetCurrentDevice API. Is this still true?

Will LTC provide a way for backend to register its counter and metrics?

Hadn't thought about this.

Right now we treat the counters' pybindings as just 'getters' (with the exception of reset). We don't actually have any API for 'configuring' the counters, bc I believe they are just configured by static macros.

I am not sure if this system will work once we combine some in-tree and external-backend code, or we will have to adapt it to 'merge' the counters from external backend together with the ones from torch::lazy:: code.

yes you can remove it. When core folks fixed convolution to flow through one op, @desertfire removed the graph checks since convolutions become just another op

I'm fine either way.

(2) can always be squished into (3) but that can make gdb debugging more annoying.

If users don't really need a python binding for 2), I'd suggest we go for it.

nit: instead of making a separate submodule, what are your thoughts on making ts_backend a submodule of _lazy? i.e. _lazy.ts_backend instead of _lazy_ts_backend

In fact, this might be what you intended all along based on its usage in test_ts_opinfo.py:
https://github.com/pytorch/pytorch/pull/74508/files#diff-222fb8e63ff517f76308939550ca744de90dd6ea5706e2afd3f72db019df9b90R24

yea its a good question;
I tried to make it clean from the actual user API side, which is how it looks when called from test_ts_opinfo.py.

generally the c++ bindings are flatter all over pytorch and then structurized at the glue-layer where .py files re-export them in a proper namespace (the stuff happening in torch/_lazy/*.py), so I didn't think it mattered too much.

I might be convinced to change it though. Wdyt @Krovatkin ? One annoying thing is I would have to then define a 'submodule' correctly in the .pyi file that provides the mypy formatting, but i'm sure that's not too hard

is the plan to expose all LTC python functions via torch._lazy? doesn't the underscore usually imply "private" and user shouldn't call? or is it eventually going to something like torch.lazy and the API exposed more similarly to how its currently done on the staging branch?

yea, torch._lazy signifies it is not a stable API, that's our convention. It can't be torch.lazy until we (as a team) make a stronger commitment to long term support, etc.