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

Re-export some submodules and add feature flags for them #759

Merged
merged 15 commits into from
Sep 4, 2023

Conversation

dae
Copy link
Contributor

@dae dae commented Sep 3, 2023

Pull Request Template

Implements some of the suggestions from #729. I think the type aliases will be useful for people new to Burn, but happy to cut them or move them if you prefer.

Is this the direction you were hoping to go with the meta crate?

Checklist

  • Confirm that run-checks script has been executed.

dae added 12 commits September 3, 2023 11:46
I'm not sure why, but the previous commit has caused this doctest to break:

---- burn-core/src/module/base.rs - module::base::Module (line 76) stdout ----
Test executable failed (exit status: 127).

stderr:
/tmp/rustdoctestUn8l2X/rust_out: error while loading shared libraries: libtorch_cpu.so: cannot open shared object file: No such file or directory

When invoked via:

cargo test -p burn-core --features test-tch,tch

Tch does still appear to work when running one of the examples
Copy link
Member

@nathanielsimard nathanielsimard left a comment

Choose a reason for hiding this comment

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

This is pretty much what I had in mind! I'm still unsure if the backend module and the autodiff re-export should be implemented in burn-core vs in burn. It might reduce the number of feature flags duplication, but won't change the end-user API.

@dae Let me know what you think is the better way, I don't have a strong preference.

Comment on lines 7 to 8
type MyBackend = WgpuBackend;
type MyAutodiffBackend = WgpuAutodiffBackend;
Copy link
Member

Choose a reason for hiding this comment

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

For this example, let's keep the generics. It's documented this way in the guide and shows how to set the element types! But having defaults is indeed convenient.

@dae
Copy link
Contributor Author

dae commented Sep 3, 2023

Because of the fact that burn-core is already using autodiff and already has the test-{backend} flags, it seems easier to keep the current setup despite the extra boilerplate routing the settings - at least it's consistent with the other exports like dataset that way. In the longer term it might be worth revisiting the crate layout - if burn-core focused on only stuff that needed to be shared between the different subcrates (or perhaps were merged into burn-common), that would make things a bit simpler when navigating around the packages.

@nathanielsimard nathanielsimard merged commit 5b97f1a into tracel-ai:main Sep 4, 2023
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.

2 participants