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

Support PyTorch Dataset for oscar #2246

Merged
merged 66 commits into from
Aug 20, 2021
Merged

Support PyTorch Dataset for oscar #2246

merged 66 commits into from
Aug 20, 2021

Conversation

yuanchongbit
Copy link
Contributor

What do these changes do?

I have done a demo of pytorch dataset API by inheriting torch.utils.data.Dataset. I also write some testcase in tests subfolder.

Related issue number#2224

I have try transfer a torch dataloader object to torch script but I have no idea how to do it. Now I am reading raySGD code to find a way to help mars to run torch or tf code more flexible.

@qinxuye
Copy link
Collaborator

qinxuye commented Jul 26, 2021

Try to use mars dataset in run_pytorch_script. This requires passing Mars objects into script.

Before Mars 0.7, we have a concept called named_tensor and named_dataframe, that is, we can name a tensor or df via obj.execute(name='sth'), then in the script users can get it by providing a name like mt.named_tensor(name='sth').

But we don't like the concept because, the named tensor/df has no difference with the vanilla one except adding a name, but we have to add more APIs, and maybe for md.index or sth, we need named_index, that's a shame. Besides, named object bypassed the lifecycle mechanism in Mars, if an object executed with a name, we don't know when to delete it. However, for vanilla objects, if it's garbage collected, the reference count on the server side will minus 1, and deleted if reference count reaches 0.

In conclusion, we need a new way to pass Mars objects between driver and pytorch script.

I suggest to add the ability to run_script, which run_tensorflow_script and run_pytorch_script rely on, like,

import mars.dataframe as md
from mars.remote import run_script

df = md.read_xxx()
run_script('/to/path/script.py', inputs=[df])

In the script.

from mars import get_context

df = get_context().get_inputs()[0]

# other parts

The hard part is context can be got inside the script, and get_inputs() is absent for context.

Updated: see #2251.

@qinxuye
Copy link
Collaborator

qinxuye commented Jul 28, 2021

#2251 has been merged to master branch, now could leverage it to support PyTorch dataset API.

@qinxuye qinxuye changed the title demo of dataset API Support PyTorch Dataset for oscar Jul 28, 2021
@qinxuye qinxuye added mod: learn to be backported Indicate that the PR need to be backported to stable branch type: feature New feature labels Jul 28, 2021
@qinxuye qinxuye added this to the v0.8.0a1 milestone Jul 28, 2021
Copy link
Collaborator

@qinxuye qinxuye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments.

mars/learn/contrib/pytorch/dataset.py Outdated Show resolved Hide resolved
mars/learn/contrib/pytorch/dataset.py Outdated Show resolved Hide resolved
mars/learn/contrib/pytorch/dataset.py Outdated Show resolved Hide resolved
mars/learn/contrib/pytorch/sampler.py Show resolved Hide resolved
mars/learn/contrib/pytorch/sampler.py Show resolved Hide resolved
mars/remote/tests/sample_script.py Outdated Show resolved Hide resolved
add zh_CN doc, add type_check in dataset and other modification
qinxuye
qinxuye previously approved these changes Aug 19, 2021
Copy link
Collaborator

@qinxuye qinxuye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@hekaisheng hekaisheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hekaisheng hekaisheng merged commit 0c9114d into mars-project:master Aug 20, 2021
@qinxuye
Copy link
Collaborator

qinxuye commented Aug 20, 2021

Congrats that this PR got merged, look forward to seeing more contributions from you.

qinxuye pushed a commit to qinxuye/mars that referenced this pull request Aug 21, 2021
@qinxuye qinxuye added backported already PR has been backported and removed to be backported Indicate that the PR need to be backported to stable branch labels Aug 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported already PR has been backported mod: learn type: feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants