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

Add manual flush #64

Merged
merged 2 commits into from
Nov 21, 2023
Merged

Conversation

evalott100
Copy link
Contributor

@evalott100 evalott100 commented Nov 6, 2023

Corresponding ioc code here

@evalott100 evalott100 self-assigned this Nov 6, 2023
@evalott100 evalott100 marked this pull request as draft November 6, 2023 16:08
@evalott100 evalott100 force-pushed the add_manual_flush branch 2 times, most recently from e7e6a2f to afe9d6a Compare November 8, 2023 08:49
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7117d68) 96.35% compared to head (3693df5) 96.43%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   96.35%   96.43%   +0.07%     
==========================================
  Files          12       12              
  Lines        1180     1205      +25     
==========================================
+ Hits         1137     1162      +25     
  Misses         43       43              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@evalott100
Copy link
Contributor Author

@AlexanderWells-diamond how do you feel about deprecating python 3.7? pandablocks-ioc is currently minimum 3.10 so I think it might be time.

Copy link
Contributor

@AlexanderWells-diamond AlexanderWells-diamond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall seems ok, but I've got a few questions that I would like answers to before approve/reject.

Also, looking at commit afe9d6a, the commit message is very confusing. It talks about adding PVs, and PVI, neither of which are anything to do with this repository?

tests/test_asyncio.py Outdated Show resolved Hide resolved
src/pandablocks/asyncio.py Outdated Show resolved Hide resolved
src/pandablocks/asyncio.py Outdated Show resolved Hide resolved
src/pandablocks/asyncio.py Outdated Show resolved Hide resolved
src/pandablocks/asyncio.py Outdated Show resolved Hide resolved
src/pandablocks/asyncio.py Outdated Show resolved Hide resolved
tests/test_asyncio.py Outdated Show resolved Hide resolved
@AlexanderWells-diamond
Copy link
Contributor

@AlexanderWells-diamond how do you feel about deprecating python 3.7? pandablocks-ioc is currently minimum 3.10 so I think it might be time.

That depends entirely on whether we want/need to keep supporting our Diamond-internal use, which is still technically on Python 3.7.2. I have no strong opinions, especially as other related repos have already broken that, but it's worth at least a moment's thought before we do so.

@evalott100
Copy link
Contributor Author

@AlexanderWells-diamond how do you feel about deprecating python 3.7? pandablocks-ioc is currently minimum 3.10 so I think it might be time.

That depends entirely on whether we want/need to keep supporting our Diamond-internal use, which is still technically on Python 3.7.2. I have no strong opinions, especially as other related repos have already broken that, but it's worth at least a moment's thought before we do so.

I think it would be a good idea to deprecate 3.7 now rather than write a workaround to have asyncio.exceptions be imported differently on it. I'd argue we should keep versions which are end-of-life around so long as they work without having to write extra code to support them.

@AlexanderWells-diamond
Copy link
Contributor

I think it would be a good idea to deprecate 3.7 now rather than write a workaround to have asyncio.exceptions be imported differently on it. I'd argue we should keep versions which are end-of-life around so long as they work without having to write extra code to support them.

Isn't just using asyncio.TimeoutError, as is done on line 156 already, a fully compatible way of referring to that exception?

You do make a fair point about end of life. My only defence of keeping using that Python version is that it's still the "standard" at Diamond.

@evalott100
Copy link
Contributor Author

I think it would be a good idea to deprecate 3.7 now rather than write a workaround to have asyncio.exceptions be imported differently on it. I'd argue we should keep versions which are end-of-life around so long as they work without having to write extra code to support them.

Isn't just using asyncio.TimeoutError, as is done on line 156 already, a fully compatible way of referring to that exception?

You do make a fair point about end of life. My only defence of keeping using that Python version is that it's still the "standard" at Diamond.

Ah yep my mistake we can go for asyncio.TimeoutError

@evalott100 evalott100 force-pushed the add_manual_flush branch 4 times, most recently from 03b1c34 to c606213 Compare November 13, 2023 10:13
@evalott100 evalott100 force-pushed the add_manual_flush branch 3 times, most recently from 439d0e0 to 16fcbb4 Compare November 13, 2023 13:38
src/pandablocks/asyncio.py Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
@evalott100 evalott100 force-pushed the add_manual_flush branch 2 times, most recently from 71dcb12 to f8b57de Compare November 13, 2023 16:13
Copy link
Contributor

@AlexanderWells-diamond AlexanderWells-diamond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code seems fine, a few possible enhancements to it. I've also got a set of comments about the generated documentation.

It's not exactly relevant to this PR, but it seems that the docs and pytest targets will no longer run on RHEL8 with Python 3.7.2. Something has bumped versions of various dependencies, that means we now see this error:

ImportError: urllib3 v2.0 only supports OpenSSL 1.1.1+, currently the 'ssl' module is compiled with 'OpenSSL 1.0.2o-fips  27 Mar 2018'. See: https://github.com/urllib3/urllib3/issues/2168

I'm uncertain if this is strictly an issue in Python 3.7, which we currently declare support for in pyproject.toml, or a specific issue to the Diamond version of Python.

src/pandablocks/asyncio.py Outdated Show resolved Hide resolved
src/pandablocks/asyncio.py Outdated Show resolved Hide resolved
src/pandablocks/asyncio.py Show resolved Hide resolved
src/pandablocks/asyncio.py Show resolved Hide resolved
src/pandablocks/asyncio.py Outdated Show resolved Hide resolved
src/pandablocks/asyncio.py Outdated Show resolved Hide resolved
tests/test_asyncio.py Show resolved Hide resolved
tests/test_asyncio.py Outdated Show resolved Hide resolved
docs/conf.py Show resolved Hide resolved
@evalott100
Copy link
Contributor Author

evalott100 commented Nov 14, 2023

I'm uncertain if this is strictly an issue in Python 3.7, which we currently declare support for in pyproject.toml, or a specific issue to the Diamond version of Python.

Thanks for the review! I'll investigate this locally.

Tested

Docs and pytest work locally for me on redhat 8.8 with python 3.7.3

@evalott100 evalott100 force-pushed the add_manual_flush branch 3 times, most recently from 1fbe1eb to 76b605b Compare November 15, 2023 13:28
Copy link
Contributor

@AlexanderWells-diamond AlexanderWells-diamond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

src/pandablocks/asyncio.py Outdated Show resolved Hide resolved
@evalott100
Copy link
Contributor Author

evalott100 commented Nov 16, 2023

@AlexanderWells-diamond We should make sure this is ready before we merge this

src/pandablocks/asyncio.py Outdated Show resolved Hide resolved
Added a new pv to change the flush mode, it can be
periodic, manual, or immediate.
Added a PVI button for it manual.
Adjusted tests to work for it.
@evalott100 evalott100 merged commit c6ab247 into PandABlocks:master Nov 21, 2023
13 checks passed
@evalott100 evalott100 deleted the add_manual_flush branch November 21, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants