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

Consolidate python3 #7

Closed
wants to merge 26 commits into from
Closed

Conversation

peteasa
Copy link

@peteasa peteasa commented Oct 9, 2024

@Lightsaver7 good points.

I have now pulled together all the commits from the three people involved so far with the python3 support.
This pull request is for the consolidate_python3 branch with my own commits plus @michaelcroquette from python3 only and @danielbrown2 from the lneuhaus repo. All merge conflicts are resolved.

The pull request also fixes issue pyrpl-fpga#505.

I have tested it with nose and tested the four different ways of starting pyrpl.

Without this pull request the nose tests will fail on your current technically working branch with python2 or python3 code because:
memory.py has a bug - see the fix in peteasa@7af8ad6
the test code pyrpl/test/test_base.py has a couple of bugs - see the fix in peteasa@13d7c09

These failure are also fixed with this pull request

Please find attached the nose test results. The one ERROR in this is again a test code failure common to both the python2 and python3 code. I have a "fix" for this however it is a complex area and I want to ensure that the "fix" is correct before submitting.

nose_stdio.log

michaelcroquette and others added 26 commits November 14, 2023 18:25
Create an anaconda environment backup file where pyrpl is working properly.

"conda env create -f environment.yml"

inside anaconda prompt to create environment
compatible with pyqtgraph=0.11 and 0.13
upadted pyqtgraph version to 0.11 -> 0.13 and pyqt to 5.9.7 -> 5.15.10
…needs to be more specific int64/float64/complex128
…work with the latest Python 3 versions. Changes also from peteasa for the async routines https://github.com/peteasa/pyrpl/tree/async_utilities
# Conflicts:
#	pyrpl/curvedb.py
#	pyrpl/hardware_modules/asg.py
#	pyrpl/hardware_modules/iir/iir.py
#	pyrpl/hardware_modules/iir/iir_theory.py
#	pyrpl/hardware_modules/iq.py
#	pyrpl/hardware_modules/pid.py
#	pyrpl/hardware_modules/scope.py
#	pyrpl/software_modules/network_analyzer.py
#	pyrpl/software_modules/spectrum_analyzer.py
#	pyrpl/test/test_hardware_modules/test_pid_na_iq.py
#	pyrpl/widgets/module_widgets/iir_widget.py
#	pyrpl/widgets/module_widgets/na_widget.py
# Conflicts:
#	pyrpl/software_modules/network_analyzer.py
# Conflicts:
#	pyrpl/software_modules/loop.py
#	pyrpl/widgets/module_widgets/iir_widget.py
#	pyrpl/widgets/module_widgets/na_widget.py
#	pyrpl/widgets/module_widgets/scope_widget.py
@michaelcroquette
Copy link

Hello ! I solved the issue with the async UI not working properly in IPython. The fix is on my py3.10_test branch. I will try to merge it with your branch and make a pull request for https://github.com/lneuhaus/pyrpl.

If you are curious it was the removal of the loop parameter from the asyncio.sleep() function that caused the issue. In python 3.10, the event loop is extracted from the get_event_loop function. But when you are in IPython environment, the get_event_loop function returns the asyncio loop which is running and not the qasync event loop.

To solve this I rewrote the sleep function to send the sleep function to the qasync event loop. It's not super clean but it works.

@peteasa
Copy link
Author

peteasa commented Oct 21, 2024

@michaelcroquette excellent.. Good luck with your merge and testing.

Note i did a meld between your code and my code and pulled in your changes locally on my machine to try it out. I might have messed that up but..

I noticed https://github.com/lneuhaus/pyrpl/blob/ae4188de932471cec6a5993bf4f504ac6c11328f/pyrpl/__init__.py#L32 does not conform to the Qt api.

I fixed that up and ran

jupyter notebook --NotebookApp.port=8754

and noticed run time errors

RuntimeError: Task <Task pending name='Task-2' coro=<Kernel.dispatch_queue() running at /usr/lib/python3/dist-packages/ipykernel/kernelbase.py:545> cb=[_wrap_awaitable.<locals>.<lambda>() at /usr/lib/python3/dist-packages/tornado/gen.py:852, IOLoop.add_future.<locals>.<lambda>() at /usr/lib/python3/dist-packages/tornado/ioloop.py:699]> got Future <Future pending> attached to a different loop
[IPKernelApp] ERROR | Error in message handler
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/ipykernel/kernelbase.py", line 545, in dispatch_queue
    await self.process_one()
  File "/usr/lib/python3/dist-packages/ipykernel/kernelbase.py", line 528, in process_one
    t, dispatch, args = await self.msg_queue.get()

Then I found the same errors when running

jupyter execute testme.ipynb

Then I succeeded to run

ipython3 testme.py

Then I succeeded to run

python3 -m nose pyrpl > nose_stdio.log 2>&1

Ran 503 tests in 638.833s
FAILED (failures=4)

I attach the nose output fyi

nose_stdio.log

RedPitaya OS 2.0: Latest Beta (2.05-37) Install 2.00-37 - Branch 2024.3 Python is 3.12.3 pyrpl 1.0.0.0 using IPython 8.20.0 numpy 1.26.4 pandas 2.1.4 paramiko 2.12.0 scipy 1.11.4 scp 0.14.5 ruamel.yaml 0.17.21 yaml 6.0.1 qtpy 2.4.1 on Ubuntu 24.04.1 LTS

@michaelcroquette
Copy link

Where is the file testme.ipynb ? I can't see it on your branch. Also I can't run the nose tests and python 3.12 because it seems nose is deprecated. I'm trying to switch to pytest but not successful at the moment.

Finally what does your pyrpl_async_utils file looks like ? There has been a lot of modifications and I'm not sure they are all judicious.

@peteasa
Copy link
Author

peteasa commented Oct 22, 2024

My main objective is to get the code working in the Ubuntu 24.04.1 environment so that I can explore both the python and the fpga and get to understand what is going on under the hood.

When you have a branch that you need tested I can easily pull it into my Ubuntu environment and run the nose tests for you if you like! - no need to waste time on test code yet - provided that there is a way to run the nose tests somehow!

I took pyrpl/test/test_ipython_notebook files and took the source and put that into a .py file and took a copy for the .ipynb file and made the modifications that I needed for hostname. I also set gui=False in both test files because I can test the gui with

python3 -m pyrpl example_filename

I was forced to create a clone of my own because every branch that I have seen seems to half do each of the jobs needed to get python3 going. I put each of the changes onto a separate branch in my clone so that I could merge each set of changes to any destination so you can look at the individual sets of changes that make up the whole if you like.

https://github.com/peteasa/pyrpl/blob/consolidate_python3/pyrpl/async_utils.py

Well my version is I agree a bit of a mess.. I looked at what everyone had in various branches and worked on one solution based on that but did not get all the invocations working.

Then I teamed up with Daniel Brown and he seemed to get the other bit working but not the bit that I had got working..

So on my consolidated branch I took his changes (including code to fix one of the issues) and my changes and used the try/ except code to figure out what the environment of the invocation was and thus was able with the INTERACTIVE flag to switch between my version and Daniel Brown's version.

Very messy but it got all the invocations working apart from launching the gui from within a ipython environment. That was enough for me because it proved that launching the code in ipython, juypiter and python3 environments and getting the gui work.

I suspect that Qt will change again so async_utils.py is highly likely to be very volatile.

Now the nose tests work ok for me and they actually identified failures that I had not seen fixed on any branch .. these failures I think must also be visible in the python2 environments .. so I added those fixes to my branch.. again you can pick up the test fixes separately if you want. So if necessary we could back port them to python2.

Next step for me is to work on the IIR functions.. I will also find why the "complicated IIR transfer functions" are failing. I have a fix that I have not published yet because I need to understand more what is happening.

I also started to look at the changes needed for PyTest.. it does not seem too difficult but I have not progressed that work yet because I think the higher priority is the IIR code.

@Lightsaver7
Copy link

Just one question. Which Python 3 version are you guys using for testing the updated version of PyRPL in the consolidated version? 3.10 or are you sticking with 3.6.8?

@peteasa
Copy link
Author

peteasa commented Oct 23, 2024

Hi,
I am using Ubuntu 24.04.1 LTS it has

$ python3 --version
Python 3.12.3

@Lightsaver7
Copy link

Thanks

@peteasa
Copy link
Author

peteasa commented Oct 26, 2024

@michaelcroquette I would be interested in your thoughts on the IIR results that I have pushed up to my testbench repository. The low / high and notch filters that I have tried to implement with the IIR module to not quite produce the results that I would expect. See https://github.com/peteasa/pyrpl_testbench/blob/main/basics/filters.ipynb

@michaelcroquette
Copy link

Hello,

So I discussed with Leonard Neuhaus (original developer of pyrpl) and he agreed to give me admin rights on the pyrpl repo so if you have a pull request that passes the tests you can submit it to the original repo.

Regarding your issues with the IIR, I had a quick look and a quick discussion with Samuel Deleglise who spent the most time debugging the IIR module. I will put my response in a discussion on your pyrpl_testbench repo.

@michaelcroquette
Copy link

Also this raises the question of whether we should work with this fork or use the original one. I will personally continue with the original repo but I'm curious to what the RedPitaya team intentions are.

My objective for now is to have a pyrpl working on the latest python distributions, bring back some sort of automatic testing and redeploy a stable version on Pypi and sourceforge so that pyrpl installation is as simple as possible. Any help is welcomed !

I have limited time for now so I'm not sure how much of that I'll be able to do but I hope to have more time at the end of january.

@peteasa
Copy link
Author

peteasa commented Nov 6, 2024

I have just spotted that the redpitaya team seem to have done some magic... they now moved the fork of https://github.com/RedPitaya/pyrpl to a fork of https://github.com/pyrpl-fpga that has all the original issues from https://github.com/lneuhaus/pyrpl so check where your copy of the lneuhaus/pyrpl is cloned from!!!

@Lightsaver7
Copy link

Hello @peteasa and @michaelcroquette,

This is great news! The team and I think it will be best if the work continues on the main pyrpl branch (as far as I can tell, both https://github.com/lneuhaus/pyrpl and https://github.com/pyrpl-fpga are now the same repository). If I had to guess, the source of the "magic" is Leonard renaming the GitHub account to "pyrpl-fpga" after giving you access, Michael. We will keep our branch in sync with the main branch, and will probably remove our repository once the main branch is working with the latest Python.

The changes that caused PyRPL to fail to work on 2.00 Red Pitaya OS, were also pushed as a pull request 518 (pyrpl-fpga#518) on the main PyRPL repository. However, you can also copy them from our branch if necessary.

If you need any help from us, please let me know (or write to [email protected]).

@michaelcroquette
Copy link

Yes no magic here sadly.

@peteasa peteasa closed this by deleting the head repository Nov 7, 2024
@peteasa
Copy link
Author

peteasa commented Nov 7, 2024

FYI I am now working towards pushing these changes to https://github.com/pyrpl-fpga

@michaelcroquette
Copy link

Great ! I've already pulled my changes from python3-only and py3.10-test to the main branch.

@Lightsaver7
Copy link

Hello @peteasa @michaelcroquette,

As a thank you for working hard on updating the PyRPL branch we would like to send you Red Pitaya Christmas cards. Would you mind giving us some contact information (the address we can send them to)? You can reach me through mail at [email protected].

Sorry to contact you like this, but I do not have any other way of getting to you :).

Best regards,
Miha

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