Tests CI workflows - Githubissues

Natooz commented 6 months ago

Following #4 , this PR set up GitHub actions to run the tests on pushes and pull requests.

Not ready for merge yet. Right now the action only perform builds as done in the "publish" action

Natooz commented 6 months ago

Also, the tests pass for 4 MIDIs out of 26, I'll try to investigate why

Yikai-Liao commented 6 months ago

There is a time limitation of github actions. So we'd better reduce some platform (like linux arm64 based on qemu and macos) and some python versions for test.

Natooz commented 6 months ago

Absolutely, apologies for this unexpected situation 😅 In the next commit I'll temporarily disable the action anyway

Natooz commented 6 months ago

@Yikai-Liao @lzqlzzq I updated the tests.yml workflow, removing all the builds parts and reducing the number of platforms and python versions. It is possible to run the tests on macOS arm runners, but as the pricing / hours multiplicator is high it might be a better idea to not test it. Should we however use QEMU with linux to test on arm64?

Hopefully the tests should run very quick, they only take ~10min for miditoolkit, here the install should be a bit longer but the rest should be fast.

Natooz commented 6 months ago

It might also be a good idea to rename the symusic directory, as it prevents to import the installed symusic module when running tests locally (from the root project directory). What do you think?

Yikai-Liao commented 6 months ago

Well, Linux containers using qemu was about 8 times slower than x64 containers (in our previous pratice). Thankfully, we don't have that many bugs to fix right now, which means the pace of releases has slowed down considerably. (I'm currently focusing on design libsymusic, which is extracted from the single header file in symusic with more friendly c++ interface and will be easier to maintain and extend)

Therefore, the test can be done without worrying too much about exceeding the total time limit of github actions. starting next month.

And by the way, my high-performance arm development board (8 cores, 16g RAM and 256g eMMC) has arrived 🎉! And I'll test symusic on it frequently.

Yikai-Liao commented 6 months ago

It might also be a good idea to rename the symusic directory, as it prevents to import the installed symusic module when running tests locally (from the root project directory). What do you think?

I agree with it. The problem is that I'm not familiar with how setup.py is actually working. And the reason why I named that folder symusic is that it can be copied to installation path directly(without renaming it). I just don't know how to achieve more complex operations.

Natooz commented 6 months ago

That's great news!

So indeed I think we can safely reduce the range of platforms / python versions of tests.

I can take a look for the setup.py behaviour, and if it's not complicated include the changes in this PR. BTW, in the future switching to a pyproject.toml package description could be considered (setup.py is legacy), if this is possible as I'm not sure the bindings are supported.

Natooz commented 6 months ago

BTW, I just updated the test, with the right sorting only 8 are failing now! 🎉 There seem to be two sorts of problems that should be easy to fix:

A mismatch between note velocity and duration. This surely comes from a FIFO error, I encountered a similar issue with miditoolkit. And that's the same error causing the last miditok test to fail;
Tempo assertion errors, with tempos being equal but the .tempo value is a fraction different, likely to come from a float type conversion error somewhere between the original MIDI format and the one written. I don't know if it can be fixed, but if it cannot I suggest to rounds .tempo when parsing a MIDI, or to do the rounding in Tempo.__equal__.

Natooz commented 6 months ago

For reference on the FIFO issue when writing a MIDI: https://github.com/YatingMusic/miditoolkit/pull/28 And how the FIFO loading was added: https://github.com/YatingMusic/miditoolkit/pull/14

Natooz commented 6 months ago

For setup.py, we just need to change the root directory name for the packages and package_dir entries. Would you like it to be done in this PR? And if so, which new name should we give to the dir?

Yikai-Liao commented 6 months ago

First, for the new folder name of symusic, I have checked some pupular repository:

repository folder name

numpy numpy

pytorch torch

mido mido

polars py-polars

matplotlib lib/matplotlib

So, what about py-symusic? (BTW, why do many libraries just use the same name as the folder name? )
Second, I have understood what's wrong with currently midi writing strategy. However, I'm still confused, becuase midi also support real time applications. If we send two note-on events in the same channel, with the same pitch (the only difference lies their velocity), then, how can we decide their order based on the given information to bring it into line with the FIFO rule? And is there any standard describing this rule in midi? Actually, it feels more like an undefined behavior to me.
Third, for the __eq__ of tempo, introduing an epsilon in compartion might cause lots of unexpected problems according to this anwser in stack overflow. At the same time, I don't find standard for bpm quantization. In short I think this issue should be handled with care.

repository	folder name
numpy	numpy
pytorch	torch
mido	mido
polars	py-polars
matplotlib	lib/matplotlib

Natooz commented 6 months ago

Ok for py-symusic. I don't why they keep the same name when the package has to be built before being tested, but they surely have some way to handle it;
Indeed there is no specific recommendation in the MIDI specifications when to it comes to the order of incoming events. mido is not FIFO if I remember well. However I think it would be good practice to at least keep a consistency between how the MIDI information is parsed and how it is written, either FIFO or LIFO, in order to keep the same information when dumping back;
Noted.

I suggest to maybe merge this branch as it comes with a lot of file changes, with minimal test platforms activated, before solving the above issues?

Yikai-Liao commented 6 months ago

I fail to merge.

refusing to allow an OAuth App to create or update workflow .github/workflows/lint.yml without workflow`scope

Natooz commented 6 months ago

I'm not sure what's the cause of this issue, if it's not solves yet I'll look into it after making sure the pytest action is run properly (8 tests should fail, right now I don't know why it cannot find the pytest command).

Yikai-Liao commented 6 months ago

It works on browser, but not on my phone

Natooz commented 6 months ago

Ok, there will still be one or two things to fix for the action to run properly.

kgantchev commented 5 months ago

It is possible to run the tests on macOS arm runners, but as the pricing / hours multiplicator is high it might be a better idea to not test it.

Might I make a suggestion? Feel free to use FlyCI's M1 and M2 runners. Our runners are on average 2x faster and 2x cheaper than GitHub's AND we have a free tier for OSS projects (see below).

Install Instructrions

Easily replace your M1 runners:

jobs:
 ci:
-    runs-on: macos-latest
+    runs-on: flyci-macos-large-latest-m1
   steps:
   - name: 👀 Checkout repo
     uses: actions/checkout@v4

Or try the M2 runners:

jobs:
  ci:
-    runs-on: macos-latest
+    runs-on: flyci-macos-large-latest-m2
    steps:
      - name: 👀 Checkout repo
        uses: actions/checkout@v4

Pricing

Processor	vCPU	RAM (GB)	Storage	Label	Price on FlyCI	Price on GitHub
M1	4	7	28 GB	flyci-macos-large-latest-m1	$0.06	-
M1	8	14	28 GB	flyci-macos-xlarge-latest-m1	$0.12	$0.16
M2	4	7	28 GB	flyci-macos-large-latest-m2	$0.08	-
M2	8	14	28 GB	flyci-macos-xlarge-latest-m2	$0.16	-

500 mins/month Free for Public Repos

If your repo is public, then FlyCI offers 500 mins/month of free M1 runner usage with the flyci-macos-large-latest-m1 runner.

Best Regards, Kiril Gantchev CEO and co-founder of FlyCI

Yikai-Liao commented 5 months ago

@kgantchev Doesn't the flyci-macos-large-latest-m1 timing need to be multiplied by a factor like github?

kgantchev commented 5 months ago

@Yikai-Liao nope... it's just 500 mins/month

Yikai-Liao commented 5 months ago

@Yikai-Liao nope... it's just 500 mins/month

Thanks, I'll try it.

Yikai-Liao / symusic

Tests CI workflows #6

Install Instructrions

Pricing

500 mins/month Free for Public Repos