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

fix(file): add example code for the cli option defined by the user #225

Merged
merged 21 commits into from
Mar 14, 2024

Conversation

YurelyCamacho
Copy link
Collaborator

Add example code for the cli option defined by the user: Argparse and Click

Solve #122
-->

How to test these changes

  • ...

Pull Request checklists

This PR is a:

  • bug-fix
  • new feature
  • maintenance

About this PR:

  • it includes tests.
  • the tests are executed on CI.
  • the tests generate log file(s) (path).
  • pre-commit hooks were executed locally.
  • this PR requires a project documentation update.

Author's checklist:

  • I have reviewed the changes and it contains no misspelling.
  • The code is well commented, especially in the parts that contain more complexity.
  • New and old tests passed locally.

Additional information

Reviewer's checklist

Copy and paste this template for your review's note:

## Reviewer's Checklist

- [ ] I managed to reproduce the problem locally from the `main` branch
- [ ] I managed to test the new changes locally
- [ ] I confirm that the issues mentioned were fixed/resolved

@YurelyCamacho YurelyCamacho requested a review from xmnlab February 28, 2024 23:55
@xmnlab
Copy link
Member

xmnlab commented Feb 29, 2024

@YurelyCamacho it seems your branch is out-of-date
could you rebase it on top of the upstream main branch please?

@YurelyCamacho YurelyCamacho marked this pull request as ready for review February 29, 2024 15:43
@YurelyCamacho YurelyCamacho requested a review from xmnlab February 29, 2024 15:43
@xmnlab
Copy link
Member

xmnlab commented Feb 29, 2024

@YurelyCamacho in gneral, it looks good to me.
could you just add smoke tests for that?
it seems we don't have yet tests for CLI. the filename could be cli.sh

you also need to to update the CI file and makim in order to include the new test

@YurelyCamacho
Copy link
Collaborator Author

@xmnlab


import argparse
import os
import sys
Copy link
Member

Choose a reason for hiding this comment

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

@YurelyCamacho YurelyCamacho requested a review from xmnlab March 1, 2024 15:22
@YurelyCamacho
Copy link
Collaborator Author

@xmnlab
Copy link
Member

xmnlab commented Mar 1, 2024

@xmnlab I don't know why this error occurred https://github.com/osl-incubator/scicookie/actions/runs/8113662218/job/22177467364?pr=225#step:5:376

this problem could happen because some parallelized usage of the ruff cache, IIRC

for now, don't worry about that, it seems that it still has an ruff issue in some file there, https://github.com/osl-incubator/scicookie/actions/runs/8113662218/job/22177467364?pr=225#step:5:377

try to fix that first, and maybe it will fix the first issue as well, of course, it would be better to have a more robust alternative to fix that ... I will try to find the configuration I saw that avoid this data racing from pre-commit

@xmnlab
Copy link
Member

xmnlab commented Mar 1, 2024

please add to the ruff config in the pre-commit-config.yaml

require_serial: yes

this PR adds some information: pre-commit/pre-commit#2437

@YurelyCamacho
Copy link
Collaborator Author

please add to the ruff config in the pre-commit-config.yaml

require_serial: yes

this PR adds some information: pre-commit/pre-commit#2437

In ruff check or ruff format?

@xmnlab
Copy link
Member

xmnlab commented Mar 1, 2024

this would be in the pre-commit hook config in the template folder: https://github.com/osl-incubator/scicookie/blob/main/src/scicookie/%7B%7Bcookiecutter.project_slug%7D%7D/.pre-commit-config.yaml#L43

there we just have for now 1 ruff command, in the future we should have both the formatter and the linter there as well

@YurelyCamacho
Copy link
Collaborator Author

this would be in the pre-commit hook config in the template folder: https://github.com/osl-incubator/scicookie/blob/main/src/scicookie/%7B%7Bcookiecutter.project_slug%7D%7D/.pre-commit-config.yaml#L43

there we just have for now 1 ruff command, in the future we should have both the formatter and the linter there as well

Sorry, I was looking at the project's repo.

@YurelyCamacho
Copy link
Collaborator Author

YurelyCamacho commented Mar 1, 2024

@xmnlab I don't know why this error occurred https://github.com/osl-incubator/scicookie/actions/runs/8113662218/job/22177467364?pr=225#step:5:376

this problem could happen because some parallelized usage of the ruff cache, IIRC

for now, don't worry about that, it seems that it still has an ruff issue in some file there, https://github.com/osl-incubator/scicookie/actions/runs/8113662218/job/22177467364?pr=225#step:5:377

try to fix that first, and maybe it will fix the first issue as well, of course, it would be better to have a more robust alternative to fix that ... I will try to find the configuration I saw that avoid this data racing from pre-commit

@xmnlab I don't know what can solve this error, any suggestions?

@xmnlab
Copy link
Member

xmnlab commented Mar 1, 2024

@xmnlab I don't know why this error occurred https://github.com/osl-incubator/scicookie/actions/runs/8113662218/job/22177467364?pr=225#step:5:376

this problem could happen because some parallelized usage of the ruff cache, IIRC
for now, don't worry about that, it seems that it still has an ruff issue in some file there, https://github.com/osl-incubator/scicookie/actions/runs/8113662218/job/22177467364?pr=225#step:5:377
try to fix that first, and maybe it will fix the first issue as well, of course, it would be better to have a more robust alternative to fix that ... I will try to find the configuration I saw that avoid this data racing from pre-commit

@xmnlab I don't know what can solve this error, any suggestions?

in the base.sh file, you can stop the script with "exit 1" before the pre-commit command ...
go to the new project folder at /tmp, activate the environment created by the test, and run pre-commit manually and check the changes made by ruff.

let me know if you need more information

@YurelyCamacho
Copy link
Collaborator Author

let me know if you need more information

Yes, I don't really understand what to do

@xmnlab
Copy link
Member

xmnlab commented Mar 1, 2024

so .. you need to debug it, so how you can do it?

you are executing a script .. so you need to break the script in a place where you can check the results and see what is happening.

where is the step where it is failing? check on the log file first to identify what is the command in the base.sh that is failing. this is really important step please take your time to identify that, every time that the CI fails you need to identify where the error is happening

after you identify that, you will find that it is happening here (please just move to this step when you really find the error in the step before) https://github.com/osl-incubator/scicookie/blob/main/tests/smoke/base.sh#L61

so before that you will need to stop the execution so you can do it by yourself and check the results. why do you need to step the execution? so the linter change the files in the (new) project folder (at /tmp/osl/osl-python-package) and you will execute the command by yourself and check the results and check what is happening. to stop the execution, just add a the instruction (exit 1) before the command that is failing

now execute the specific test that is failing, if you have doubts, check the logs and try to find which test is failing, TIP: in the logs, when a line starts with + generally it means that it is a command that is executed, not a output, but also the tests has some outputs that will indicate which test is executed.

so after you execute the specif test that is failing, it will break in the point you added exit 1

now go to the new project created at /tmp/osl/osl-python-package and activate also the conda environment conda activate osl-python-package. now run the linter command .. it will fail .. and now you can check what is the result after the changes and compare with the files in the template folder.


does it make more sense now? let me know if that helped.
also, please after this PR, create a new section in the documentation about how to debug the tests with this information :)

@YurelyCamacho YurelyCamacho force-pushed the 122-cli-option branch 3 times, most recently from 201d3ee to 2f9297a Compare March 9, 2024 00:09
@YurelyCamacho
Copy link
Collaborator Author

@xmnlab Done, please review. It remains to create a new section in the documentation on how to debug the tests with the information you mentioned.

import click

from {{ cookiecutter.package_slug }} import __version__
{%- if cookiecutter.use_black == "yes" %}
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain a bit this black specific workflow?

As far as I understood, this is used by to add 2 empty lines, correct? If so, I guess that could be used also when black is not used.

Could you give more details about tgat please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Initially I did it this way because it gave me a black error because I didn't have the 2 empty lines. Let me test if I remove the black condition, how the tests behave.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@xmnlab It worked without the condition for black. I was trying several things here until it worked. Can you check again? Thanks

@YurelyCamacho
Copy link
Collaborator Author

@xmnlab Did you check that I removed the condition for black? I don't understand what is still missing?

@YurelyCamacho
Copy link
Collaborator Author

@xmnlab That's the change you told me about, right?
At first I did it that way because of the suggestion you gave me to put in chatGPT the argparse function and see a similar one for click. And that (including if __name__ == "__main__": app()) was what chatGPT gave me, that's why I had only placed it for Click.

@xmnlab
Copy link
Member

xmnlab commented Mar 13, 2024

yep .. that is what I was explaining .. now it has some issue there ... not sure yet .. but maybe you need to remove the cli.py if the the cli option is None ...

@xmnlab
Copy link
Member

xmnlab commented Mar 13, 2024

@YurelyCamacho YurelyCamacho requested a review from xmnlab March 14, 2024 01:17
@xmnlab xmnlab merged commit 1b1db90 into osl-incubator:main Mar 14, 2024
12 checks passed
@xmnlab
Copy link
Member

xmnlab commented Mar 14, 2024

thanks @YurelyCamacho

@YurelyCamacho YurelyCamacho deleted the 122-cli-option branch March 14, 2024 14:20
Copy link

🎉 This PR is included in version 0.7.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants