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

Extract wasm logic from DVCClient into a new wrapper #152

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

rob-odwyer
Copy link
Contributor

This refactors the DVCClient so that the WASM and object pool related code is now contained inside a new wrapper type, WASMLocalBucketing.

@rob-odwyer
Copy link
Contributor Author

rob-odwyer commented Apr 10, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

SetClientCustomData(customData []byte) error
Variable(user DVCUser, key string, variableType string) (variable Variable, err error)
Close()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm probably going to need to tweak this interface so the serialization of the user and custom data only happens for the WASM flow. I think that's only needed for passing that data safely to WASM?

Copy link
Member

Choose a reason for hiding this comment

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

only really needed for passing data safely - and performance - but we should be fine moving to just a raw go type for params if we're slowly deprecating WASM fully

client_native.go Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing these in this PR to make the tests pass cleanly without the native-specific client methods being fully implemented, but I'll add them back again in the next PR.

client_wasm.go Outdated
Copy link
Contributor Author

@rob-odwyer rob-odwyer Apr 10, 2023

Choose a reason for hiding this comment

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

Fun fact - this filename didn't work because suffixing with _wasm tells Go to only compile it when building for the "wasm" arch, which is one of the supported targets of the compiler.

Copy link
Member

Choose a reason for hiding this comment

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

suffixes are hard

@rob-odwyer rob-odwyer marked this pull request as ready for review April 10, 2023 21:38
@rob-odwyer rob-odwyer force-pushed the DVC-6914-extract-wasm-from-client branch from edbf57b to fb5033c Compare April 11, 2023 20:28
@rob-odwyer rob-odwyer changed the title extract wasm logic from DVCClient into a new wrapper Extract wasm logic from DVCClient into a new wrapper Apr 11, 2023
@rob-odwyer rob-odwyer merged commit eb4bcbb into main Apr 11, 2023
@rob-odwyer rob-odwyer deleted the DVC-6914-extract-wasm-from-client branch April 11, 2023 21:27
JamieSinn pushed a commit that referenced this pull request Apr 14, 2023
extract wasm logic from DVCClient into a new wrapper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants