-
Notifications
You must be signed in to change notification settings - Fork 6
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
Update base project config files #228
Conversation
@jkbhagatio Why not consolidate the dependency installment into
and then a user can only run e.g., |
@JaerongA ah yeah sure, I could add a gpu section to pyproject.toml, so pip install could be done directly from pyproject.toml, good point. I'd still keep the env yml files because afaik there's no way to conda/mamba install from a pyproject.toml? I guess I can delete the requirements*.txt files though? |
@jkbhagatio Sounds good.. yeah, it may only install from pip. Not sure how it can be tweaked to install from conda. |
WalkthroughThe proposed changes primarily focus on enhancing the development environment setup and introducing new workflows for better code management. The updates include new tools for auto-formatting, code checks, and CI integration. GitHub Actions workflows are introduced to automate merging and squashing commits from 'main' to 'prod', and for building the environment, running checks, tests, and generating coverage reports. The Python version is updated to python3.11 and pre-commit hooks are updated with new ones added. A new file for common preprocessing functions is also added. Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Files selected (9)
- .flake8 (1)
- .github/workflows/build_env_run_tests.yml (1)
- .github/workflows/gpt_pr_review.yml (1)
- .github/workflows/squash_merge_to_prod.yml (1)
- .pre-commit-config.yaml (2)
- aeon/io/reader.py (1)
- env_config/.python-version (1)
- readme.md (1)
- requirements.txt (1)
Files ignored due to filter (5)
- env.yml
- env_config/env.yml
- env_config/env_dev.yml
- env_config/env_gpu.yml
- pyproject.toml
Files not summarized due to errors (9)
- .github/workflows/gpt_pr_review.yml (nothing obtained from openai)
- .flake8 (nothing obtained from openai)
- aeon/io/reader.py (nothing obtained from openai)
- .github/workflows/build_env_run_tests.yml (nothing obtained from openai)
- .github/workflows/squash_merge_to_prod.yml (nothing obtained from openai)
- .pre-commit-config.yaml (nothing obtained from openai)
- env_config/.python-version (nothing obtained from openai)
- requirements.txt (nothing obtained from openai)
- readme.md (nothing obtained from openai)
Files not reviewed due to errors (9)
- .github/workflows/gpt_pr_review.yml (no response)
- .flake8 (no response)
- .github/workflows/squash_merge_to_prod.yml (no response)
- .pre-commit-config.yaml (no response)
- aeon/io/reader.py (no response)
- .github/workflows/build_env_run_tests.yml (no response)
- readme.md (no response)
- requirements.txt (no response)
- env_config/.python-version (no response)
Review comments generated (0)
- Review: 0
- LGTM: 0
Tips
Chat with CodeRabbit Bot (@coderabbitai
)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitai
in a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignore
anywhere in the PR description to pause further reviews from the bot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Files selected (2)
- .github/workflows/gpt_pr_review.yml (1)
- .github/workflows/squash_merge_to_prod.yml (1)
Files skipped from review due to trivial changes (1)
- .github/workflows/squash_merge_to_prod.yml
Review comments generated (1)
- Review: 0
- LGTM: 1
Tips
Chat with CodeRabbit Bot (@coderabbitai
)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitai
in a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignore
anywhere in the PR description to pause further reviews from the bot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Files selected (4)
- .github/workflows/build_env_run_tests.yml (1)
- .github/workflows/squash_merge_to_prod.yml (1)
- .pre-commit-config.yaml (2)
- aeon/preprocessing/init.py (1)
Files ignored due to filter (2)
- env_config/env_dev.yml
- pyproject.toml
Files skipped from review due to trivial changes (2)
- .github/workflows/squash_merge_to_prod.yml
- aeon/preprocessing/init.py
Review comments generated (15)
- Review: 13
- LGTM: 2
Tips
Chat with CodeRabbit Bot (@coderabbitai
)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitai
in a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignore
anywhere in the PR description to pause further reviews from the bot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Files selected (1)
- .github/workflows/build_env_run_tests.yml (1)
Files ignored due to filter (2)
- pyproject.toml
- setup.cfg
Review comments generated (5)
- Review: 5
- LGTM: 0
Tips
Chat with CodeRabbit Bot (@coderabbitai
)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitai
in a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignore
anywhere in the PR description to pause further reviews from the bot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly okay, left a few comments for consideration and we can discuss further at the meeting. I have also renamed the PR for clarity. Happy to do a different title but just "Config" is too terse to include in release notes.
|
||
# Only run codebase checks and tests for ubuntu. | ||
- name: ruff | ||
if: matrix.os == 'ubuntu-latest' && matrix.python-version == '3.11' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we replace all instances of string 3.11
below with an environment variable to make it easier to change the python version for all the steps in the future?
It should be possible to do this using workflow environment variables: https://docs.github.com/en/actions/learn-github-actions/variables#defining-environment-variables-for-a-single-workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup good idea, easy magic number to avoid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, actually no! Looks like env variables are only supported for steps
, but not for the strategy
subfields in an actions workflow: https://stackoverflow.com/questions/74072206/github-actions-use-variables-in-matrix-definition
+1 code-rabbit =p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkbhagatio ah, failed to notice that, but we can still to get the values inside using matrix variables.
From the updated file it looks like you already did that right? So it should be all good now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkbhagatio Another option, if we are really not going to run tests on any other python version in the matrix, would be to simply remove the checks altogether no? Python is already setup correctly using conda_incubator
so why not just assume that these checks are not necessary?
.github/workflows/gpt_pr_review.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How long does it take to run this workflow? I still have mixed feelings about leaning on this even if only to generate PR summaries and recommendations. It failed to notice easy opportunities to refactor, such as the environment variables comment I made above, and my fear is always that this will discourage people from reading the code changes in detail.
At least on my end I will still do a full pass review by hand, but I guess if everyone is happy I am also happy to ignore it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mm it was pretty quick, within a couple minutes, faster than build_env_run_tests
my fear is always that this will discourage people from reading the code changes in detail.
For me at least I'd actually say the opposite - it will probably make me read the code in more detail because I'll read it anyway and then probably pay extra attention to its suggestions.
Happy to revisit discussion on it in future after we see more examples of what it comes up with, but at least today people seemed intrigued : )
docs/devs/readme.md
Outdated
- Ensure dev dependencies are installed in your Aeon Python environment | ||
- Ensure pre-commit hooks are installed in your Aeon Python environment: run `pre-commit install` in the activated environment | ||
- Use [pyan3](https://github.com/Technologicat/pyan) for generating callgraphs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we elaborate here on what we are supposed to do with the output of pyan3
? I can see how call graphs might be useful when reading the code base in some cases, but do we need to generate them when developing? Or is it more of a general tooling recommendation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was more of a reminder to myself. I found pyan when I wanted to generate diagrams for callgraphs for readers.
It's just a random point, but I can move it, any thoughts on where it should live?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, decided just to remove this point from here (well now from 'contributing.md', given I've deleted this file)
docs/devs/readme.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if this README file should be renamed as CONTRIBUTING.md? Seems like these are guidelines for anyone wanting to contribute to the repo. If we use the standard GitHub naming convention it seems like GH will automatically add links to relevant places, e.g. issues and PRs: https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/setting-guidelines-for-repository-contributors#about-contributing-guidelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there, just a few minor suggestions left.
|
||
# Only run codebase checks and tests for ubuntu. | ||
- name: ruff | ||
if: matrix.os == 'ubuntu-latest' && matrix.python-version == '3.11' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkbhagatio Another option, if we are really not going to run tests on any other python version in the matrix, would be to simply remove the checks altogether no? Python is already setup correctly using conda_incubator
so why not just assume that these checks are not necessary?
aeon/io/reader.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a change is still being introduced here in that the white line was entirely removed. Maybe just revert the file to the version in the previous commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option, if we are really not going to run tests on any other python version in the matrix, would be to simply remove the checks altogether no?
yep, can just remove the python-version checks here
pyproject.toml
Outdated
description = ''' | ||
Code for managing acquired data from Project Aeon experiments. Includes general file IO, | ||
data QC, querying, and analysis modules. | ||
''' | ||
authors = [ | ||
{ name = "Jai Bhagat", email = "[email protected]" }, | ||
{ name = "Goncalo Lopes", email = "[email protected]" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change my email here to g.lopes AT neurogears.org?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, done
Sounds good, just the email change missing? |
It shows up here for me |
These look good to me now, but maybe check what's up with |
Yep the tests are failing on Ubuntu because ruff doesn't approve of formatting, and the build failing on macos is related to a DJ wheel not being built properly - something @ttngu207 is looking into |
@jkbhagatio pyright still seems to be missing from pyproject.toml if I get this right? Might be easier to open both yml and toml side by side and make them identical. Can conda bootstrap from pyproject.toml too? Would be preferable to have a single source of truth for dependencies. |
Oops, will fix that. No unfortunately, conda can't use pyproject as of now |
Updated base config files.
Please update your aeon python env! See the env*.yml files or requirements*.txt files or pyproject.toml file depending on what you use to manage your aeon python env!
Biggest noticeable dev changes will be pre-commit running auto-formatting and checks via black, ruff, and pyright.
Also github actions CI will run on pushes to some branches, and PRs to 'main'.
Summary by CodeRabbit
@coderabbitai: ignore