-
Notifications
You must be signed in to change notification settings - Fork 101
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
Set compiled_modules=False
automatically
#539
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #539 +/- ##
==========================================
- Coverage 85.22% 83.93% -1.29%
==========================================
Files 39 39
Lines 2342 2347 +5
==========================================
- Hits 1996 1970 -26
- Misses 346 377 +31 ☔ View full report in Codecov by Sentry. |
Also I think the default |
Co-authored-by: Miles Cranmer <[email protected]>
Co-authored-by: Miles Cranmer <[email protected]>
it's "auto" now |
I think that's how you set a default? I don't see any example using that final argument of |
Co-authored-by: Miles Cranmer <[email protected]>
How many CIs are expected to pass? |
All the tests that run should pass. I think the "Required" ones should be turned off as they don't run anymore. |
Not sure what the git errors are from but this error seems to be real: https://github.com/JuliaPy/pyjulia/actions/runs/6748661679/job/18347465730?pr=539 |
This would be breaking, so we're going to need a version bump. |
it's not breaking though. Before it errors, now it warns. Performance regression is not breaking. Besides, if you're working with Python getting things to work is 1st priority? |
So one example is PySR has logic for catching the default unsupported python error and then turning off compiled_modules with some extra instructions on how to install pyenv. Not sure what other packages may be affected. |
that's precisely why we want this cuz you're already doing it -- again, being able to use something (in both direction) is better than nothing |
Oh yeah I completely agree! Just was saying that this is worth a 0.6 -> 0.7 since it is a change in behavior and would require downstream changes to accommodate |
it might make your switching logic in pysr dead code, but it shouldn't break it? |
Right but other packages may use it differently |
I feel that relying on error and do something non-trivial (other than recovering like pysr) is a bit weird, but okay yeah technically it's possible |
You've changed the default behavior with respect to |
I still don't see how it's breaking, but I don't really care about the semantics debate. We should just try to get this out |
is our CI broken? |
@Moelf I think I got it working - was just a test for the docs which failed, because they didn't include the new Could you perhaps add a test with a static library so we can verify it works? For example like this: https://github.com/MilesCranmer/PySR/blob/f178813c1f8aaefd2a19006dd4d24b645d9eaf76/.github/workflows/CI.yml#L110-L138 – because conda has a static python library so would normally fail this test. |
Can do, but why do half of the tests still failing? |
The failures look real. This is the test name: |
See, for example:
|
You could add
and I think that should solve it? |
fix #487
@MilesCranmer