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

Simplify initialization #18

Closed
wants to merge 5 commits into from
Closed

Simplify initialization #18

wants to merge 5 commits into from

Conversation

Korijn
Copy link
Collaborator

@Korijn Korijn commented Dec 24, 2019

I felt like the backend import mechanism is a good fit for our needs internally, but harder than it needs to be for users. So I left the import mechanism intact, but provided an init function on the root module for users. Advantages:

  • Users can simply import wgpu and call wgpu.init() to get started right away, without needing to make a choice of backend (Rust is the default). I think this will be just fine for the majority of users.
  • Users can still pick a specific backend by providing the backend kwarg to init. Example: wgpu.init(backend='js')

(I also removed os.environ["RUST_BACKTRACE"] = "0" since that is something both developers and users should be able to set on their environment, rather than in code.)

@Korijn Korijn changed the title wgpu.init Simplify initialization Dec 24, 2019
@almarklein
Copy link
Member

I agree that an init() call is more user-friendly than an import. But the current way does have one big advantage: it avoids dynamic imports, and therefore allows tools like PyInstaller to work without further guidance. That said, only few users will freeze apps, so maybe we should indeed go for init(). One can still use the import approach when writing a freeze script. What do you think?

@almarklein
Copy link
Member

Also, we may call init() automatically if requestadapter gets called and no backend is selected yet. Another line less in user code.

@Korijn
Copy link
Collaborator Author

Korijn commented Dec 25, 2019

But the current way does have one big advantage: it avoids dynamic imports, and therefore allows tools like PyInstaller to work without further guidance. That said, only few users will freeze apps, so maybe we should indeed go for init(). One can still use the import approach when writing a freeze script. What do you think?

You're right. That sucks.

Well, we can always contribute a hook to the pyinstaller repo to make it work out of the box.

Also, we may call init() automatically if requestadapter gets called and no backend is selected yet. Another line less in user code.

Great idea! Just like matplotlib's use(backend) function, opt-in. :)

@almarklein
Copy link
Member

A similar discussion arguably goes for the gui backend.

I just experimented with this a bit. By explicitly importing wgpu.qt, it will included that module and its dependencies (Qt). When importing wgpu.tk, it will that module instead, and its dependencies (tk), no Qt libs in the frozen app. Same should go for the API backend (rs vs ..).

we can always contribute a hook to the pyinstaller repo to make it work out of the box.

I don't think that will help, because we still need a way to specify what backend to include, so this will probably be the end-user juggling includes and excludes.

What about:

  • We always support selecting gui/api backens by importing them.
  • We document somewhere that this is what ppl should do when using PyInstaller.
  • We add a hook in requestAdapter() that imports the default api backend when nothing is selected when it's called.
  • We don't need an init() function just yet.
  • When/if we get more API backends, we can add the init() function, and keep all the above intact.

@Korijn
Copy link
Collaborator Author

Korijn commented Dec 28, 2019

Sounds good.

@Korijn
Copy link
Collaborator Author

Korijn commented Jan 2, 2020

I'll reject this PR, since no changes are needed given your latest suggestion.

I created #25 to track pyinstaller support.

@Korijn Korijn closed this Jan 2, 2020
@Korijn Korijn mentioned this pull request Jan 2, 2020
6 tasks
@almarklein
Copy link
Member

We'd still need a hook in requestAdapter(), right?

@almarklein almarklein deleted the init-function branch January 2, 2020 21:59
@Korijn
Copy link
Collaborator Author

Korijn commented Jan 2, 2020

That wouldn't be pyinstaller compatible though. I'm not sure how I feel about that... :/ it feels like setting the user up for a gotcha

@almarklein
Copy link
Member

We can document what's needed to freeze an app containing wgpu. And in that code inside requestAdapter() that does the import, we can catch ImportError and provide a useful error message. That said, you have a point that wgpu will behave differently when frozen, and this will cause some tears for someone at some point. Let's discuss this in person.

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