-
Notifications
You must be signed in to change notification settings - Fork 0
Minimal pip install (local editable), dependency updates, & management
#7
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
pip install (local editable), dependency updates, & management
|
Thank you @mbarlow12 ! @rjplevin , @lloyd-rmi , is this something you could review and, if appropriate, merge? |
pyproject.toml
Outdated
| ] | ||
| requires-python = ">=3.11" | ||
| readme = "README.md" | ||
| license = { text = "MIT License" } |
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.
@mbarlow12 If I understand correctly, this is implying that the entire repo is under the MIT license. If so, that's not the case, unfortunlately. But it looks like the pyproject spec supports 'license-file' as well as 'license'--so we can just point to the hybrid MIT-Stanford license that's in the repo? Also, that way if we're ever able to change the license, using a pointer means this will always refer to whatever is current. If that's the case, can we make that change, to use license-file rather than license?
|
For some reason test_graph.py fails for me locally, before I have even made any changes to the PR. It seemed to run fine on the automated tests so I won't block on it, but should figure out why there's a difference there. |
It was mentioned in PR#7 that the renaming of these stream contents could require further XML changes. Since there is uncertainty here and there is currently no harm in keeping the generic stream names, I'm reversing the renaming for now. This should be revisited when we're confident that the name changes won't result in any unknown knock on effects.
|
Windows test is failing when trying to install conda, but I assume that's not an issue because we're getting rid of our conda usage anyways. |
opgee/etc/opgee.xml
Outdated
|
|
||
| <Stream src="HeavyOilUpgrading" dst="ProductionBoundary"> | ||
| <Contains>gas</Contains> | ||
| <Contains>gas process</Contains> |
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.
Are there no consequences to changing the wording here? My assumption is that this controls the naming of streams as they are going through the model so updating the naming here is only relevant for identifying a specific stream during debugging/logging, but has no other effect, correct?
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.
Added a commit to reverse this for now simply because there's still a bit of uncertainty around any potential knock on effects and this renaming isn't strictly needed within this PR. Can be revisited later when we can confirm this renaming doesn't affect anything unexpectedly
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.
(Hmm... github marked this as "Outdated", but I see comments from 28 minutes ago.)
The stream content tags are used by processes to distinguish among input / output streams, so it's a key runtime feature, not just for debugging/logging. In general, changes to stream contents must be coordinated with the processes at both ends of the stream. The exception is if each end expects only 1 stream and doesn't look for it by name.
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.
Outdated because I added a commit to reverse the name changes here as they weren't critical to this PR. Still something that we can certainly do, but I just wanted to take away any uncertainty while Michael is OOO this week.
Note
This branch was originally built to integrate with wrapgee to allow RMI folks to easily install OPGEEv4 via
pip install -e packages/opgee. Other fixes came about as various bugs were encountered.@mbarlow12 is OOO next week, please feel free to make push updates to the
rmi-pip-installbranch and approve/merge when ready.Description
pyproject.tomlwith full PEP 621 metadata and move to the modern PEP 517 build backend; legacysetup.py,setup.cfg, andrequirements.inare removed. Requirements are now generated withuv pip compile, producing a single, lock-stylerequirements.txt.DownholePump: correct ordering ofmultiply_flow_ratesvs. fugitive-addition.TransmissionCompressor/HeavyOilUpgrading: rename stream contents from plain “gas” to explicit “exported gas” / “gas process”.Stream.flow_rateand several TODO comments around boundary handling, acid-gas splitting, emissions categorisation.PostProcessor.decache()fixture prevents state bleed between tests.test_processes.py; value shifted from 1520.21852 → 1494.86053 mmbtu/day after dependency upgrades.Motivation and Context
pip install -r requirements.txt -e .without relying on Conda or legacysetup.py.Types of changes (check all that apply)
Change Summary
uv; bump many libs to 2025-series versions.src/layout, prune obsolete PDM & GitHub-Actions files.Additional notes
Commit List
field_to_xml.dotbugrun