-
Notifications
You must be signed in to change notification settings - Fork 8
Upgrade and un-pin dependencies and Python version to use #289
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #289 +/- ##
===========================================
+ Coverage 71.07% 71.14% +0.07%
===========================================
Files 44 44
Lines 5809 5844 +35
Branches 1147 1158 +11
===========================================
+ Hits 4129 4158 +29
- Misses 1359 1364 +5
- Partials 321 322 +1 ☔ View full report in Codecov by Sentry. |
There's one notebook test still failing when building the documentation. I'm on it. |
And I've just gifted you a merge conflict 🙃. Sorry! |
...except my PR broke everything so we'll have to revert it anyway 🤷 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! I've tried it with Python 3.12 and it works happily 🥳
I've stuck some minor comments/questions throughout, but on the whole this seems sensible. The repetition of the drop_vars()
stuff looks a bit clunky to me though I'm not sure if there's a nicer way of doing it... Are we putting columns in the dataframe that shouldn't really be in there?
Besides that, I'm wondering if we need to update the publish.yml
workflow. I'm guessing that we only need a single wheel for all the Python versions because we don't have any compiled code in here... right? But also we probably want to bump the Python version of the pyinstaller-built binary to 3.12 so it's a bit faster for end-users.
Before merging this we need to decide on the release strategy. The inputs, outputs and functionality are the same (or they should), but we are dropping support for Python 3.8, which might annoy people. Also, it will be good if someone with domain knowledge could check the output results of the examples and the tutorials. As I said in the PR description, changes to the output files are very small and, except in a couple of cases, limited to a reordering of the columns or rows, but with the same overall content, so I do not expect non-sensical results, but would be worth checking. @ahawkes , could anyone from your side have a look at this? |
@dalonsoa is there an easy way for me to see the results from the examples and tutorials? |
I've merged this into my documentation branch, so you see the results from the tutorials here (bearing in mind that there are still issues with tutorials 2 and 7 which are unrelated to these changes). I've given it a quick glance, and as far as I can tell all of the figures look identical compared to how they were before (to see how they were before merging these changes you can download the documentation artifacts from here). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell this all looks good! I've tested it with 3.9 and 3.12 at it works fine. The changes to the output files look quite small, and all the graphs in the documentation look the same as they were before.
I have to give some benefit of the doubt as it's a bit too massive to check every line, but I can't see any glaring issues, so I'm happy to approve.
Yes, this looks good - thanks!
…________________________________
From: Tom Bland ***@***.***>
Sent: 11 June 2024 08:57
To: EnergySystemsModellingLab/MUSE_OS ***@***.***>
Cc: Hawkes, Adam D ***@***.***>; Mention ***@***.***>
Subject: Re: [EnergySystemsModellingLab/MUSE_OS] Upgrade and un-pin dependencies and Python version to use (PR #289)
This email from ***@***.*** originates from outside Imperial. Do not click on links and attachments unless you recognise the sender. If you trust the sender, add them to your safe senders list<https://spam.ic.ac.uk/SpamConsole/Senders.aspx> to disable email stamping for this address.
@tsmbland approved this pull request.
As far as I can tell this all looks good! I've tested it with 3.9 and 3.12 at it works fine. The changes to the output files look quite small, and all the graphs in the documentation look the same as they were before.
I have to give some benefit of the doubt as it's a bit too massive to check every line, but I can't see any glaring issues, so I'm happy to approve.
—
Reply to this email directly, view it on GitHub<#289 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AC37JLINYAWBV4EMOMZXKJTZG2UYPAVCNFSM6AAAAABHFDPHF6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBZGYZTOMBZG4>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Ok, so if everyone is happy we merge and create a release. I'd wait until #328 is done and then merge and release first thing tomorrow morning. |
I don't suppose a simple solution is possible to #319 and could be in this release too? Possibly not... |
I'm in no rush to merge this PR, so happy to wait as long as needed. @alexdewar is on that issue, but I'm not sure of his implementation timeline. I think he was tackling #317 first. |
Yep, I'm working on #317 right now, but I'm hoping to have a chance to look at #319 this week too. Personally, I don't think we have to issue a new release as soon as we merge this branch. I think it would be ok to merge it, fix these small issues then make the release. The advantage of merging this sooner rather than later is that we'll have a chance to use the code over the next week or so and so there'll be more opportunities to notice bugs before we make a release (although it seems fine). |
Sounds good!
…________________________________
From: Diego Alonso Álvarez ***@***.***>
Sent: 11 June 2024 12:04
To: EnergySystemsModellingLab/MUSE_OS ***@***.***>
Cc: Hawkes, Adam D ***@***.***>; Mention ***@***.***>
Subject: Re: [EnergySystemsModellingLab/MUSE_OS] Upgrade and un-pin dependencies and Python version to use (PR #289)
This email from ***@***.*** originates from outside Imperial. Do not click on links and attachments unless you recognise the sender. If you trust the sender, add them to your safe senders list<https://spam.ic.ac.uk/SpamConsole/Senders.aspx> to disable email stamping for this address.
Ok, so I'll merge this tomorrow morning but will hold on the release until #317<#317>, #319<#319> and #328<#328> are done, hopefully by the end of the week. Correct?
—
Reply to this email directly, view it on GitHub<#289 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AC37JLJ3BBMG43D7T3MFFC3ZG3KSRAVCNFSM6AAAAABHFDPHF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRQGQ3DKMJTHA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Alea iacta est! |
Description
Update the whole codebase and tests to enable MUSE running with more modern Python versions,
pandas
andxarray
. So far, due to the versions used ofpandas
andxarray
we were limited to using Python 3.9, at most.This PR is massive, as the changes to be made were all or nothing. A broad summary of the changes made scattered across the code are:
timeslice
-related ones, where all levels now need to be dropped and assigned explicitly.Other changes:
Things to do:
pyproject.toml
Fixes #120
Type of change
Please add a line in the relevant section of
CHANGELOG.md to
document the change (include PR #) - note reverse order of PR #s.
Key checklist
$ python -m pytest
$ python -m sphinx -b html docs docs/build
Further checks