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

Dont use __init__ #300

Open
rafaqz opened this issue Apr 27, 2023 · 14 comments
Open

Dont use __init__ #300

rafaqz opened this issue Apr 27, 2023 · 14 comments
Labels
enhancement New feature or request no stale Don't let this issue be auto-closed

Comments

@rafaqz
Copy link

rafaqz commented Apr 27, 2023

Given recent precompilation concerns it would be better not to use and recommend using __init__ here.

Instead pynew() could have a runtime value check, and do the import automatically on the first call?

We could pass it a string for the module name, or a closure.

@rafaqz rafaqz added the enhancement New feature or request label Apr 27, 2023
@cjdoris
Copy link
Collaborator

cjdoris commented Apr 28, 2023

Nice idea.

Would be good to investigate if adding such a check has any performance penalty, since it will be done for every python operation.

@cjdoris cjdoris closed this as completed Apr 28, 2023
@cjdoris
Copy link
Collaborator

cjdoris commented Apr 28, 2023

Of course if any package does anything with PythonCall (even indirectly) in its __init__ then the effect is undone. But we can at least recommend against it.

@rafaqz
Copy link
Author

rafaqz commented May 6, 2023

Was this actually implemented?

@cjdoris cjdoris reopened this May 9, 2023
@cjdoris
Copy link
Collaborator

cjdoris commented May 9, 2023

No, I guess I hit the Close button by mistake!

@cjdoris cjdoris reopened this May 9, 2023
@pedromxavier
Copy link
Contributor

@cjdoris is there any benchmark suite of yours? I'd like to test a few a ideas for such lazy-loading mechanism.

@cjdoris
Copy link
Collaborator

cjdoris commented May 15, 2023

Just https://github.com/cjdoris/PythonCall.jl/blob/main/BENCHMARKS.md which may be out of date. Might be nice to formalise them.

@github-actions
Copy link
Contributor

This issue has been marked as stale because it has been open for 30 days with no activity. If the issue is still relevant then please leave a comment, or else it will be closed in 7 days.

@github-actions github-actions bot added the stale Issues about to be auto-closed label Aug 20, 2023
@github-actions
Copy link
Contributor

This issue has been closed because it has been stale for 7 days. You can re-open it if it is still relevant.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 28, 2023
@LilithHafner
Copy link
Contributor

IMO this is still relevant, it should be re-opened and added to a milestone so that it is not automatically re-closed as stale.

@rafaqz
Copy link
Author

rafaqz commented Sep 21, 2023

Auto closing issues after 30 days doesnt really make sense, Im not going to keep pinging all my githib issues

@cjdoris cjdoris added no stale Don't let this issue be auto-closed and removed stale Issues about to be auto-closed labels Sep 21, 2023
@cjdoris
Copy link
Collaborator

cjdoris commented Sep 21, 2023

Don't worry, pinging every 30 days is not the intention - it's to let actual stale issues die and prompt something to happen with inactive issues, even if the something is simply to add a label that prevents it from being marked stale again, which I have just done.

That said, this was marked stale because of inactivity. I'm unlikely to work on this myself so if you want anything to happen you need to make it happen.

@cjdoris cjdoris reopened this Sep 21, 2023
@rafaqz
Copy link
Author

rafaqz commented Sep 21, 2023

Marking stale due to inactivity is a questionable practice. Issues that are not fixed don't just go away.

There is no chance I will work on this, it was basically a "heads up" issue for future design.

@cjdoris
Copy link
Collaborator

cjdoris commented Sep 21, 2023

Like I said, issues which are actual issues I can label so they stick around, like I did with this one.

The bot is to close the issues that aren't actually issues - such as because it actually got resolved or the user didn't provide a way to reproduce the issue.

@rafaqz
Copy link
Author

rafaqz commented Sep 22, 2023

Issues can be closed from PRs using e.g. closes #x . Or manually if they dont matter.

But auto closing will end up with discussions like this - there is a reason almost no other packages do it. But your call of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request no stale Don't let this issue be auto-closed
Projects
None yet
Development

No branches or pull requests

4 participants