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

add lib versioning #1

Merged
merged 1 commit into from
Mar 25, 2021
Merged

add lib versioning #1

merged 1 commit into from
Mar 25, 2021

Conversation

cakester
Copy link

@cakester cakester commented Mar 22, 2021

Checklist

GitHub

  • I've given this PR a concise, self-descriptive, and meaningful title
  • I've linked relevant issues in the PR body
  • I've applied the relevant labels to this PR
  • I've assigned a reviewer

PR contents

Description

Linked issues

ivadomed#501

@cakester cakester requested a review from dyt811 March 22, 2021 16:18
requirements.txt Outdated
@@ -1,22 +1,22 @@
bids-neuropoly>=0.2
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally, i think all >= should be ~= on the safe side

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. I think this would be a very valid issue to bring up as potentially first issue. There MAY have been a reason but I concur with your assessment that it is usually safer to ~= whenever possible. ADS had some FIELD days with their conda environment management process (axondeepseg/axondeepseg#452 and axondeepseg/axondeepseg#484). We might eventually leverage conda at IvadoMed too but that is another discussion. So far, there appear to not be strong need... but that PyTorch requirement... may pose problems down the road. We will keep an eye out.

A minor feedback is to make sure use complete English sentence when interacting with IvadoMed developers. Also... everything you write today WILL be remembered by GitHub for decades to come, so make sure to make them count professionally and impressive over the years.

@cakester
Copy link
Author

note the versions are gotten from https://github.com/ivadomed/ivadomed/runs/2181168099?check_suite_focus=true, not pip freeze, since some of the newer versions ci couldn't find

@@ -8,6 +8,7 @@ on:
push:
branches:
- master
- hanna
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be mindful this doesn't get pushed to the main repo. I know you added this to test it here.
Also, for the branch name, please ensure you follow what others have done in terms of branch name see below, with prefix for addressing specific issues if relevant.
image

Copy link

@dyt811 dyt811 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rock and roll, all good to go. 🚀

@dyt811
Copy link

dyt811 commented Mar 25, 2021

Don't forget to properly Squash and Merge with the appropriate commit messages summarizing everything.

@dyt811
Copy link

dyt811 commented Mar 25, 2021

Also, as a general feedback for commit messages, feel free to go into a bit more details/contexts and references if needed. Usually, if you are not sure you are being verbose or being too brief compare to what is expected of you, lean cautiously towards the verbose side to ensure the communication is extra clear.

@dyt811
Copy link

dyt811 commented Mar 25, 2021

Also, for the real PRs to IvadoMed, make sure you checkmark all the relevant tasks completed from the prompt. You can check mark it after PR issued but best to do it at issue time via [x].

@cakester cakester force-pushed the hanna branch 2 times, most recently from 82b0b1f to 2ae8d9c Compare March 25, 2021 14:04
@cakester cakester merged commit 5e402fe into master Mar 25, 2021
@cakester cakester mentioned this pull request Mar 25, 2021
7 tasks
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