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

Updated packaging format #10

Closed
wants to merge 11 commits into from
Closed

Conversation

ZviBaratz
Copy link
Contributor

Created setup.cfg and pyproject.toml files, moved package directory to src/, applied black formatting. Resolves #9.

Created setup.cfg and pyproject.toml files, moved package directory to
src/, applied black formatting.
@ZviBaratz ZviBaratz mentioned this pull request May 17, 2022
Includes scikit-sparse in requirements to provide the Cholesky decomposition
solver. Resolves Deep-MI#13.
@m-reuter
Copy link
Member

Thank you for this pull request and your work on this. We appreciate your efforts and really like your changes with respect to the updated docstrings, the introduction of types, and the overall (toml) architecture. However there are several edits that would need to be reverted in order for this PR to become useful:

  • In the toml file black and flake8 (as dependency and their config) is very special to your setup and would need to be removed.
  • Some files appear to be deleted and recreated in another folder (e.g. Solver.py), while others (e.g. Conformal.py) are simply moved (at least on GitHub when viewing the changed files). This is problematic as the diff for these files is lost, in contrast to those that were moved. This makes it hard for us to assess any potential changes within these files. If possible treat all files the same and make sure that they are moved and diffs show correctly.
  • Code formatting lines: a line width of 79 characters is too restrictive, and we’d like to avoid multi-line wraps such as ll. 428 ff. in src/lapy/Solver.py, for example. Further your lint formatter breaks in strange places (leaving closing brackets on their own line etc). So please remove the automated line breaking. In some places line splits look good (e.g. in Functions with parameter initialisations) and can stay.
  • Code formatting quotes: also we do not care about single or double quotes. Developers can decide what they prefer. So those don't have to be changed. In fact we slightly prefer single quotes, to make it easier to use " inside (which is more common than the other way around). But we don't really care.
  • configurations.py: constants that are not used across functions / modules such as ta00, ta11, tb, … in src/lapy/configuration.py should stay local in their respective files (src/Solver.py in this case). Since only the default config is left, we would like to revert the configurations.py file completely.
  • messages.py: similarly most messages are very specific, and should stay in their original files / functions, also to avoid too much switching between files during development. Please revert both messages and config files. There are also no plans to support other languages than english.
  • Your gitignore contains many paths that are special to your setup. Optimally we try to avoid polluting the code with personal settings.

Will send a similar reply to your other PR in brainprint. And thanks again!

@mscheltienne
Copy link

I would like to give short feedback on this PR as I am discovering both tools LaPy and FastSurfer, and as it relates to #16.
@ZviBaratz If you don't want to continue this PR, I don't mind picking it up and addressing the comments from @m-reuter.

  • black and flake8 are not specific to a setup. They are listed as dev dependencies, which can be installed with the additional keyword. Code style dependencies are common and usually enforced in CI or even in pre-commit. I would argue you could also add codespell, isort, pydocstyle and mypy. Those tools are here to improve the overall code quality and readability.
  • The line width of 79 characters is indeed restrictive. This now old standard has been replaced and is not the default black uses. black defaults to 88, and other projects tend to use 100.
  • black: code formatting quotes, code format, line length.. black brings a convention, a standard to how a python code should look like: fix line-length, line breaks in a certain reproducible way, double quotes, and so on.. It's now adopted by more and more projects (IIRC numpy, scikit-learn, ..) and is improving code readability. The standard is also very easy to follow as a simple command black folder will blackify all the python files in the given folder. More and more IDEs also have black available to auto-format files, removing the need for the command mentioned.
  • And finally, the point relating to Update .gitignore with a complete Python version, update packaging and add workflows #16, the codebase should indeed not be polluted by personal settings, but the .gitignore should capture files generated by the most common IDEs and by editable installation, commonly used by developers. Else, uncommitted files are constantly present in the directory tree. As an example, here is the .gitignore from numpy and from scikit-learn.

My 2c, from a fellow developer.

@ZviBaratz
Copy link
Contributor Author

Hi @mscheltienne, thank you for your comment. I actually created this PR following a PR in another project from that lab (Deep-MI/BrainPrint#5, please see for more context). I agree with everything you wrote, I'll just add that if you decide to continue to work on this and single quotes are for some reason a hard requirement, you can also use blue. In any case, I will not be working on this further, you are very welcome to pick things up from here.

@mscheltienne
Copy link

OK. Also I now realize that both this package and BrainPrint-python are not released on Pypi or a conda distribution channel. IMO, it's a shame to not distribute those 2 packages widely.. and it would be very quick to do so.

@ZviBaratz
Copy link
Contributor Author

Again, I entirely agree, but I am not sure what are the authors' intentions. My impression from the comments here and in the other PR were that these tools are developed exclusively by and for the purposes of that particular lab. Without the core maintainers' support in implementing Python best practices and promoting collaborative development, I would recommend either adopting a different approach or working independently.

@m-reuter
Copy link
Member

Hi, thanks for you valuable comments. It is our intention to support collaborative development and we are very happy about your support! We did initially not agree with all your edits, some of which are style design decisions that we did not share. If there is good reason (e.g. standards that are adopted by other large packages) we are happy to revise our position.
Also it would be great to have Pypi and/or conda distributions.

@ZviBaratz
Copy link
Contributor Author

@m-reuter glad to hear, and thank you.
In that case, @mscheltienne please let me know if you need help with anything or need in clarifications picking up where I left off.

@mscheltienne
Copy link

mscheltienne commented Feb 22, 2023

I merged this branch into #16 to keep the changes from @ZviBaratz and its contribution in the git tree, but I did change the packaging/configuration slightly compared to this PR. The changes are now in #16 which supersedes this PR.

  • I removed the nesting of the package in a src directory. This is common practice for other programming languages, but not so much for Python. See for instance numpy or scipy .
    This change also fixes your comment "Some files appear to be deleted and recreated in another folder", since they are now back in the original folder.

  • There are 3 main ways to package a python library: setup.py (now legacy), setup.cfg and pyproject.toml. setup.cfg is still very popular, but it is more and more replaced by the newer pyproject.toml standard which I used here.

  • I added CI workflows for:

    • build (not so important for pure Python packages, but it makes sure that a package is correctly installed from the different sources)
    • code-style, running different style checker among which flake8, black, isort which format the code in a standard way; codespell which helps to correct some typos, pydocstyle to enforce numpy-style documentation.
    • pytest to run unit tests
    • publish to release the project on Pypi. This workflow is for now configured with a manual trigger, but ideally you wuold enable the trigger on 'published new release'. It also requires a PYPI_API_TOKEN secrets, I can give you detail instruction on how to generate one and where to save it later, it's very simple.
  • Changes to the actual code:

    • isort sorting out imports (there is a standard order 😄) and black formatting the code.
    • an util development tool with an entry-point lapy-sys_info to display information about the system, the interpreter, the dependencies (inspired from MNE-Python)
    • an util to manage optional dependencies, sksparse in this case (inspired from pandas), used in the Solver.
  • I removed configurations.py and messages.py as requested by @m-reuter and actually reverted changes to the code except the one mentioned above, bringing the focus of those 2 PRs on packaging.


With those changes, the pure packaging aspects are covered and the library can be released easily on pip and conda-forge. Since they are no unit tests, I invite you to give a shot to the library as it is in #16 to confirm that everything is working, you can install it with pip install git+https://github.com/mscheltienne/LaPy/tree/gitignore.

One more point, all the changes here aim at both improving the user experience by offering an easier install via pip and conda, while keeping or even reducing maintenance. With the workflows, cutting a new release is as easy as using the "Create a new release" feature of GitHub, and then letting the workflows do the release on Pypi and on conda-forge.

Finally, further changes which are not related to packaging could be added in a future PR, e.g.

  • refactoring of the doc strings as started in this PR by @ZviBaratz since the current doc strings do not comply with conventions (my IDE is full of DXXX warnings 😅). Usually, I use the numpy convention.
  • After the docstrings are updated, the pydocstyle workflow could be enable and a documentation build could be added, generating a website from the code (and the doc strings) and deploying it automatically on pushes to the main branch.
  • Replacing prints with logger outputs
  • Moving the examples to a sphinx-gallery generated webpage

If you want, I don't mind opening those subsequent PR, transforming this repository and project further. You will then be able to use this repository as a 'template' to follow good practices and conventions for future projects of your lab.

Apologies for the long post, please ask any question you might have.

@m-reuter
Copy link
Member

Wow, thanks so much for these fast edits. We will take a look. It would be great to automate the release in the future. Also the future PR ideas sound great.

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.

Update packaging format
3 participants