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

CI build workflows #9

Merged
merged 18 commits into from
Aug 25, 2022
Merged

CI build workflows #9

merged 18 commits into from
Aug 25, 2022

Conversation

andreped
Copy link
Contributor

What's changed:

  • I have implemented two workflows which create binary installers of RootPainter3D for Windows and Ubuntu.

Install works fine and the program launches. However, I am unable to further test it, as I observed that the software only supports .nii.gz and .nrrd, but I had files with .nii extension. Probably good to add support for that as well, as it is quite common (without the compression, I know).

Added two snapshots of the software running after installation on Win10 using the binary installer created built on the cloud using GitHub Actions.

I believe this is stable enough to be merged with the master branch. We can built upon the CI stuff and test if it works. I am uncertain whether pytorch works well, as I have not tested it with PyInstaller before. So that is something we would have to test.

Skjermbilde 2022-08-24 205610
Skjermbilde 2022-08-24 205406

@andreped
Copy link
Contributor Author

Oh, I just noted that you had made some parts of the windows install stuff:
https://github.com/Abe404/RootPainter3D/blob/master/painter/build/assets/Installer.nsi

However, I believe what I made works well, and both for Windows and Ubuntu. But if you want to, you can move around the stuff I made, to where you would prefer it to be.

But it would be great if you merged first and then did the necessary modifications.

@Abe404
Copy link
Owner

Abe404 commented Aug 24, 2022

Thanks, this is really excellent. I noticed that you plan to update the pull request tomorrow to include the related Mac Installer parts. I look forward to testing this and am sure the updated pull request can be merged.

I have one request. Is it possible for you to clearly specify the license for the source code you contribute by adding a GPL header to the files that you add.

See here for examples of how I currently do this:
https://github.com/Abe404/RootPainter3D/blob/master/trainer/trainer.py
and https://github.com/Abe404/RootPainter3D/blob/master/trainer/main.py

It is not 100% required but where useful, I would also appreciate a one-liner (or more) briefly explaining the function of each file in the doc-string at the top of the file. I don't enforce this strictly so add at your own discretion. The GPL header is required though, to make it clear that you are happy for your contribution to then go under this license.

Thanks for your contribution!

@Abe404
Copy link
Owner

Abe404 commented Aug 24, 2022

Oh, I just noted that you had made some parts of the windows install stuff: https://github.com/Abe404/RootPainter3D/blob/master/painter/build/assets/Installer.nsi

However, I believe what I made works well, and both for Windows and Ubuntu. But if you want to, you can move around the stuff I made, to where you would prefer it to be.

But it would be great if you merged first and then did the necessary modifications.

Yes, there is already some installer code in the project. I think it's OK for us to switch to your way of doing though as I am particularly interested in the CI component. If you have a bunch of trouble with dependencies then we might want to switch back to my way of doing it.

I will add an issue regarding .nii files

EDIT:

I had completely forgot that I had already copied across this installer system (from RootPainter to RootPainter3D). Sorry I should have let you know earlier. Anyway, still happy to go with your approach and delete the old stuff (if everything works fine) and push on with the CI etc.

@andreped
Copy link
Contributor Author

andreped commented Aug 25, 2022

The macOS CI stuff ran successfully:
https://github.com/andreped/RootPainter3D/runs/8010931845?check_suite_focus=true

So now we have CIs for all relevant operating systems.


The CI stuff can be used with any method for creating installers really. If we find that what I created did not work (or we are unable to get it work) we can use your build method. No worries. We can just integrate that into the CI. But I believe it should work. If not, there is likely some hooks that are missing, but using CIs, using PyInstaller for making executables, and making binary installers in the way I did it is extremely generic and something that I have used in lots of projects. Have never had issues with it. The only one I was unsure about was torch's compability with PyInstaller. This we will have to test.

@andreped andreped mentioned this pull request Aug 25, 2022
@Abe404
Copy link
Owner

Abe404 commented Aug 25, 2022

Thanks!

If I could make a small request it would be to add a short paragraph in the readme explaining the CI/build process to new devs who might want to get involved in the project and understand what is going on. Basically the paragraph should explain to new devs with little experience how they would go about building installers for all three platforms (which specific comments to execute, even if this is just to say that they can commit and push a repo) and then where they would find the created installers that they could then download and distribute.

Additionally, for any newly created files I would be interested in seeing a GPL header in the comments at the top where that is possible, just to make the license for use of the code super clear. See #9 (comment)

If you are not OK with this GPL header then we can also discuss mixed license options but in that case it might be cleaner to have stuff as separate modules pulled in via pip or something.

@andreped
Copy link
Contributor Author

a short paragraph in the readme explaining the CI/build process to new devs who might want to get involved in the project and understand what is going on.

As I am busy for the next couple of days, can you make this an issue and I can fix this after work on Friday?

More importantly, we should update the usage, as one does not need to clone and run the python code anymore, but rather download the release. However, this requires that we make a new release and make the compiled binary installers available in it. But I would also think we should properly test that this workflow works first. I would assume there would be some issues with training and whatnot.

If you are not OK with this GPL header

I am fine with GPL, but I don't have time to do this now. However, I can do it on Friday. But I would still merge this, such that you could start testing the binary installers and all that. And if you find issues you can fix that directly, without doing it in my fork.

@Abe404
Copy link
Owner

Abe404 commented Aug 25, 2022

This work is really cool

I am also a bit occupied with other tasks for the rest of today so lets come back to it Friday or next week some time.

I also like your idea for unit tests, although I have had mixed results when using them for projects like this that could be argued are still in the prototype stage (sometimes a lot can change and break many unit tests causing a lot of work).

I would also like to add a hook for static analysis (pylint) but I need to spend some time familiarizing myself with what you have done so far.

Thanks again.

@andreped
Copy link
Contributor Author

I would still merge this into the master, as the CI stuff does not break the current install setup method described in the README.

It also makes it easier for you to test and debug the workflow if something is breaking.

I don't see a point in defining a license for the CI-related scripts (.yml and .spec). They are derived from MIT-licensed software. So I would prefer to keep these open. But your the boss :)

We should schedule a meeting after you have tested the binary installers, where we could talk about the solution and you could help me get started using RootPainter3D.

I have a cool application which we could try. Video annotation, but these videos are normally stored in a different format (.mhd/.raw).

@Abe404
Copy link
Owner

Abe404 commented Aug 25, 2022

To keep it simple (in my perspective) my preference is to make everything we can GPL (in this project) and use comments in the headers of every new file that make that clear. Could you please also include the copyright information in the header and also where relevant a link back to the original project/repo.

I'm also not 100% sure if the spec files should have a license. But my request is that you add the discussed header anyway as they could be considered part of the software and I would like to make it clear what rights people have to copy, modify or distribute.

It seems it is OK to convert MIT code to GPL code: see https://softwareengineering.stackexchange.com/a/105926

My understanding is that this would only be for the modified versions of the scripts that we use within this repo. The originals would stay as MIT licensed.

Regarding MIT vs GPL I am not totally against starting or working on MIT projects but as this project is already GPL, I think it's simpler to use that and avoid a mixed license situation where possible.

I would prefer to get this copyright/license issues cleared up before merging anything.

Kind regards

@Abe404
Copy link
Owner

Abe404 commented Aug 25, 2022

and the video annotation application sounds super cool! I look forward to discussing more soon. I'm not sure U-Net is the best architecture for this but I need to read more about it.

@Abe404
Copy link
Owner

Abe404 commented Aug 25, 2022

I should have provided more clarity on how to contribute earlier on. I will add something to the readme to make it clearer.

@andreped
Copy link
Contributor Author

If any of the code is GPL-licensed, everything has to have the same license, as GPL is a "virus" license. Same applied if people want to use parts of our code in new softwares. Their software becomes infected, which is exactly why I don't like it. However, I am fine with having GPL license for this project.

@andreped
Copy link
Contributor Author

I have added GPL license to everything now, even the hooks. So should be no problems.


BTW: Not everything needs its own hook file. Most of the time PyInstaller finds the relevant hooks itself. It is just that some packages don't have an open hook file in this repo:
https://pypi.org/project/pyinstaller-hooks-contrib/2021.5/

Which is what PyInstaller uses. Might be that some are missed. That we can see when we start testing the software.

@Abe404
Copy link
Owner

Abe404 commented Aug 25, 2022

Thanks for updating the headers.

I noticed the copyright header is not correct. But I can fix it if after merging if that's fine with you?

Basically it should say
Copyright (C) 2022 André Pedersen

For the new files as these were written by you.

@Abe404 Abe404 merged commit 6404754 into Abe404:master Aug 25, 2022
@andreped
Copy link
Contributor Author

I don't want copyright on it as I just want it to be completely public to anyone xd But lets just keep you on copyright, as you are the owner of the repo, if you prefer to have GPL license.

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.

2 participants