EnergySystemsModellingLab / MUSE_OS

Welcome to the MUSE-OS repository
https://muse-os.readthedocs.io/en/latest/
GNU General Public License v3.0
22 stars 8 forks source link

Upgrade and un-pin dependencies and Python version to use #289

Closed dalonsoa closed 2 weeks ago

dalonsoa commented 1 month ago

Description

Update the whole codebase and tests to enable MUSE running with more modern Python versions, pandas and xarray. So far, due to the versions used of pandas and xarray 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:

Other changes:

Things to do:

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

Further checks

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 71.14%. Comparing base (af65733) to head (d3b4f44).

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dalonsoa commented 3 weeks ago

There's one notebook test still failing when building the documentation. I'm on it.

alexdewar commented 3 weeks ago

And I've just gifted you a merge conflict :upside_down_face:. Sorry!

alexdewar commented 3 weeks ago

...except my PR broke everything so we'll have to revert it anyway :shrug:

dalonsoa commented 3 weeks ago

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?

ahawkes commented 3 weeks ago

@dalonsoa is there an easy way for me to see the results from the examples and tutorials?

tsmbland commented 2 weeks ago

@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).

ahawkes commented 2 weeks ago

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 listhttps://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 GitHubhttps://github.com/EnergySystemsModellingLab/MUSE_OS/pull/289#pullrequestreview-2109637097, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC37JLINYAWBV4EMOMZXKJTZG2UYPAVCNFSM6AAAAABHFDPHF6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBZGYZTOMBZG4. You are receiving this because you were mentioned.Message ID: @.***>

dalonsoa commented 2 weeks ago

Ok, so if everyone is happy we merge and create a release. I'd wait until https://github.com/EnergySystemsModellingLab/MUSE_OS/pull/328 is done and then merge and release first thing tomorrow morning.

ahawkes commented 2 weeks ago

I don't suppose a simple solution is possible to #319 and could be in this release too? Possibly not...

dalonsoa commented 2 weeks ago

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.

alexdewar commented 2 weeks ago

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).

dalonsoa commented 2 weeks ago

Ok, so I'll merge this tomorrow morning but will hold on the release until #317, #319 and #328 are done, hopefully by the end of the week. Correct?

ahawkes commented 2 weeks ago

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 listhttps://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 #317https://github.com/EnergySystemsModellingLab/MUSE_OS/issues/317, #319https://github.com/EnergySystemsModellingLab/MUSE_OS/issues/319 and #328https://github.com/EnergySystemsModellingLab/MUSE_OS/pull/328 are done, hopefully by the end of the week. Correct?

— Reply to this email directly, view it on GitHubhttps://github.com/EnergySystemsModellingLab/MUSE_OS/pull/289#issuecomment-2160465138, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC37JLJ3BBMG43D7T3MFFC3ZG3KSRAVCNFSM6AAAAABHFDPHF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRQGQ3DKMJTHA. You are receiving this because you were mentioned.Message ID: @.***>

dalonsoa commented 2 weeks ago

Alea iacta est!