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

Set compiled_modules=False automatically #539

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Moelf
Copy link

@Moelf Moelf commented Nov 3, 2023

Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (69b734f) 85.22% compared to head (48f1b8e) 83.93%.
Report is 7 commits behind head on master.

❗ Current head 48f1b8e differs from pull request most recent head d35b213. Consider uploading reports for the commit d35b213 to get more accurate results

Files Patch % Lines
src/julia/core.py 0.00% 5 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

src/julia/core.py Outdated Show resolved Hide resolved
@MilesCranmer
Copy link
Collaborator

Also I think the default compiled_modules should then be "auto". Otherwise the default will trigger errors anyways.

@Moelf
Copy link
Author

Moelf commented Nov 3, 2023

it's "auto" now

@Moelf
Copy link
Author

Moelf commented Nov 3, 2023

I think that's how you set a default? I don't see any example using that final argument of Choices :(

src/julia/options.py Outdated Show resolved Hide resolved
Co-authored-by: Miles Cranmer <[email protected]>
@Moelf
Copy link
Author

Moelf commented Nov 4, 2023

How many CIs are expected to pass?

@MilesCranmer
Copy link
Collaborator

All the tests that run should pass. I think the "Required" ones should be turned off as they don't run anymore.

@MilesCranmer
Copy link
Collaborator

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

@mkitti
Copy link
Member

mkitti commented Nov 6, 2023

This would be breaking, so we're going to need a version bump.

@mkitti mkitti added the breaking label Nov 6, 2023
@Moelf
Copy link
Author

Moelf commented Nov 6, 2023

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?

@MilesCranmer
Copy link
Collaborator

Before it errors, now it warns

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.

@Moelf
Copy link
Author

Moelf commented Nov 6, 2023

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

@MilesCranmer
Copy link
Collaborator

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

@Moelf
Copy link
Author

Moelf commented Nov 6, 2023

it might make your switching logic in pysr dead code, but it shouldn't break it?

@MilesCranmer
Copy link
Collaborator

Right but other packages may use it differently

@Moelf
Copy link
Author

Moelf commented Nov 6, 2023

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

@mkitti
Copy link
Member

mkitti commented Nov 6, 2023

You've changed the default behavior with respect to compiled_modules. That's fine, but I may prioritize non-breaking changes for merging and release.

@Moelf
Copy link
Author

Moelf commented Nov 6, 2023

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

@Moelf
Copy link
Author

Moelf commented Nov 30, 2023

is our CI broken?

@mkitti mkitti closed this Dec 1, 2023
@mkitti mkitti reopened this Dec 1, 2023
@Moelf Moelf closed this Dec 29, 2023
@Moelf Moelf reopened this Dec 29, 2023
@MilesCranmer
Copy link
Collaborator

@Moelf I think I got it working - was just a test for the docs which failed, because they didn't include the new "auto" option.

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.

@Moelf
Copy link
Author

Moelf commented Jan 1, 2024

Can do, but why do half of the tests still failing?

@MilesCranmer
Copy link
Collaborator

The failures look real. This is the test name: test_compatible_exe.py::test_incompatible_python. I guess they just need to be updated to the new behavior.

@MilesCranmer
Copy link
Collaborator

See, for example:

assert "It seems your Julia and PyJulia setup are not supported." in proc.stderr

@MilesCranmer
Copy link
Collaborator

You could add compiled_modules=True to the line

Julia(runtime=os.getenv("PYJULIA_TEST_RUNTIME"), debug=True)

and I think that should solve it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Turn on compiled_modules=False automatically?
3 participants