-
Notifications
You must be signed in to change notification settings - Fork 0
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
Release of 4.0.8 #166
Release of 4.0.8 #166
Conversation
✅ Deploy Preview for pyra-4-documentation ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
@patrickjaigner do you want to have a look at it before we try to run it on system ma? |
dde489a
to
c4323fa
Compare
Had to rebase it onto the main branch, that is why I force-pushed. |
try: | ||
_run_pyra_core_lock.acquire() | ||
_run_pyra_core_lock.release() | ||
except filelock.Timeout: |
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.
Is this the best way to do this?
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.
I think so.
run_pyra_core.py
will acquire that lock before the mainloop starts and frees it when the program finishes or crashes:
Lines 14 to 22 in 52eb662
if __name__ == "__main__": | |
try: | |
_run_pyra_core_lock.acquire() | |
try: | |
main.run() | |
finally: | |
_run_pyra_core_lock.release() | |
except filelock.Timeout: | |
print("Aborting start: core process is already running") |
When the pyra-cli start
command cannot acquire the Lock, some Pyra Core process must be running.
The other option would be to cycle through all running processes (like the is-running
command), but I think that is more work to implement because there are different cases.
self.config["general"]["min_sun_elevation"] - 1 | ||
) * utils.Astronomy.units.deg | ||
|
||
if current_sun_elevation is not None: |
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.
Why could you remove this line?
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.
I could remove this because the new Astronomy class implementation will throw an exception when the current sun elevation is not a float. I return float(output from skyfield library)
:
pyra/packages/core/utils/astronomy.py
Lines 60 to 69 in 52eb662
current_position = earth + skyfield.api.wgs84.latlon( | |
latitude_degrees=lat, | |
longitude_degrees=lon, | |
elevation_m=alt, | |
) | |
sun_pos = current_position.at(current_time).observe(sun).apparent() | |
altitude, _, _ = sun_pos.altaz() | |
return float(altitude.degrees) |
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.
There is also a new test that draw random datetimes from each year between 2020 and 2050 and checks if the Astronomy class can generate an elevation for that: https://github.com/tum-esm/pyra/blob/integration-4.0.8/tests/utils/test_astronomy.py
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.
Could you let me know why you changed the Docstrings? Now they are different lengths and also every shorter than the rest of the code.
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.
Sure! The wide comment blocks are hard to read on small monitors. I often work with two files open next to each other:
But when the lines of the doc strings are very long, it is super annoying when every lines wraps to a new line:
On big monitors, that is not an issue but whenever using two files or doing any diffing stuff on a tiny one, it bothers a lot.
Would you like to revert it?
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.
Didn't realize this, but sure let's keep the changes.
# logger.debug(f'Successfully closed OPUS instance "{p}"') | ||
# except Exception as e: | ||
# logger.warning(f"Failed to close OPUS: {e}") | ||
|
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.
I think we should not keep in in the code, but move it to the issue.
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.
I finished reading through the changes. There is a lot of good refactoring done. I also added some comments for lines where I needed clarification on why changes were made.
Overall we should think about how easy and accessible the tum-esm-utils library is. Will people easily find the API description (a quick Google search didn't give me the correct result) or will it confuse people as relevant code is now moved to a different repository? I'm unsure if I like it for Pyra, as it will make it harder for people to add additional things to functions or make changes. What do you think?
EDIT: Regarding the documentation of the utility library, from demo.mp4 |
@dostuffthatmatters Finished my review. All points clarified, thank you. |
52eb662
to
e4e1425
Compare
@patrickjaigner this is running well on the integration station. Do you approve the release? |
@dostuffthatmatters Sounds good. Let's confirm a 7 days stable run of the release. Other than that I agree that 4.0.8 is ready to go. |
Integration still in progress:
4.0.7
tum-esm-utils
more #165