search

UBC-MDS / software-review-2021

1 stars 1 forks source link

Submission : labzen (Python) #10

Open sukh2929 opened 3 years ago

kmoravej commented 3 years ago

Submitting Author:

Package Name: labzen One-Line Description of Package: A package to help MDS students better manage their lab assignments. Repository Link: https://github.com/UBC-MDS/labzen Version submitted: 0.1.18 Editor: TBD
Reviewer 1: TBD
Reviewer 2: TBD
Archive: TBD
Version accepted: TBD

Description

  • labzen is a Python package that adds more zen to your student experience of working on MDS labs. It lets you manage common tasks such as counting total marks in an assignment, and performs common checks for mechanics in your iPython notebooks and R markdown assignments.

Scope

* Please fill out a pre-submission inquiry before submitting a data visualization package. For more info, see notes on categories of our guidebook.

Technical checks

For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:

Publication options

JOSS Checks - [ ] The package has an **obvious research application** according to JOSS's definition in their [submission requirements][JossSubmissionRequirements]. Be aware that completing the pyOpenSci review process **does not** guarantee acceptance to JOSS. Be sure to read their submission requirements (linked above) if you are interested in submitting to JOSS. - [ ] The package is not a "minor utility" as defined by JOSS's [submission requirements][JossSubmissionRequirements]: "Minor ‘utility’ packages, including ‘thin’ API clients, are not acceptable." pyOpenSci welcomes these packages under "Data Retrieval", but JOSS has slightly different criteria. - [ ] The package contains a `paper.md` matching [JOSS's requirements][JossPaperRequirements] with a high-level description in the package root or in `inst/`. - [ ] The package is deposited in a long-term repository with the DOI: *Note: Do not submit your package separately to JOSS*

Are you OK with Reviewers Submitting Issues and/or pull requests to your Repo Directly?

This option will allow reviewers to open smaller issues that can then be linked to PR's rather than submitting a more dense text based review. It will also allow you to demonstrate addressing the issue via PR links.

Code of conduct

P.S. *Have feedback/comments about our review process? Leave a comment here

Editor and Review Templates

Editor and review templates can be found here

cmmclaug commented 3 years ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

It is understood that reviewers and authors are all members of the same UBC MDS 2021 cohort.

Documentation

The package includes all the following forms of documentation:

Readme requirements The package meets the readme requirements below:

The README should include, from top to bottom:

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider:

Functionality

For packages co-submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing: 3

Review Comments

Hey labzen team,

Congratulations on a clever and creative project idea! With about 80 labs in the program by my estimate, this is definitely something your target audience can relate to and use! After going through the code, this was certainly an ambitious project, and I commend your hustle. I apologize for having to report any negative feedback, but hopefully I can provide another set of eyes to help polish the package!

Requirements

I was unable to check off all of the package requirements as described below:

1.

[ ] Vignette(s) demonstrating major functionality that runs successfully locally

I was unable to get the check_mechanics function to work as described in the vignette. I tried to execute the commands in the Python interpreter, but there is no provided token for the sample inputs. The .gif is a nice touch, but there is no mention of JupyterLab in the package and I went off script to reproduce there, but was also unsuccessful. Finally, I tried with a personal repo, but received the following error as described below:

>>> pyfile = "lab1.ipynb"
>>> reponame = "DSCI_563_lab1_cswag7"
>>> lz.check_mechanics(reponame, pyfile)
Enter a valid token generated from github.ubc.ca to get the details from remote: 11224~RqHgE5WjOgFWcGaDIaj0iAJCsMLaaNtCuJ4LBh3bIEPXC4y2cIXjTuD5cGbb5V30
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/miniconda3/lib/python3.8/site-packages/labzen/labzen.py", line 426, in check_mechanics
    check_lat_version(repo_name),
  File "/opt/miniconda3/lib/python3.8/site-packages/labzen/labzen.py", line 280, in check_lat_version
    org = g.get_organization("MDS-2020-21")
  File "/opt/miniconda3/lib/python3.8/site-packages/github/MainClass.py", line 311, in get_organization
    headers, data = self.__requester.requestJsonAndCheck("GET", "/orgs/" + login)
  File "/opt/miniconda3/lib/python3.8/site-packages/github/Requester.py", line 315, in requestJsonAndCheck
    return self.__check(
  File "/opt/miniconda3/lib/python3.8/site-packages/github/Requester.py", line 340, in __check
    raise self.__createException(status, responseHeaders, output)
github.GithubException.BadCredentialsException: 401 {"message": "Bad credentials", "documentation_url": "https://docs.github.com/enterprise/2.22/rest"}
>>>

2.

[ ] Any additional setup required (authentication tokens, etc)

The first additional setup note required would be an authentication token to execute the check_mechanics function as noted above.

After package installation I ran poetry install to install any package dependencies I may have been missing. Unfortunately, I received the following the following error message:

(base) craigm@MacBook-Pro-135:~/mds/block5/dsci524/labzen$ python
Python 3.8.8 | packaged by conda-forge | (default, Feb 20 2021, 16:12:38) 
[Clang 11.0.1 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from labzen import labzen as lz
Traceback (most recent call last):
  File "/opt/miniconda3/lib/python3.8/site-packages/gitdb/__init__.py", line 20, in _init_externals
    __import__(module)
ModuleNotFoundError: No module named 'smmap'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/miniconda3/lib/python3.8/site-packages/git/__init__.py", line 25, in _init_externals
    import gitdb
  File "/opt/miniconda3/lib/python3.8/site-packages/gitdb/__init__.py", line 28, in <module>
    _init_externals()
  File "/opt/miniconda3/lib/python3.8/site-packages/gitdb/__init__.py", line 22, in _init_externals
    raise ImportError("'%s' could not be imported, assure it is located in your PYTHONPATH" % module)
ImportError: 'smmap' could not be imported, assure it is located in your PYTHONPATH

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/craigm/mds/block5/dsci524/labzen/labzen/labzen.py", line 9, in <module>
    import git
  File "/opt/miniconda3/lib/python3.8/site-packages/git/__init__.py", line 33, in <module>
    _init_externals()
  File "/opt/miniconda3/lib/python3.8/site-packages/git/__init__.py", line 27, in _init_externals
    raise ImportError("'gitdb' could not be found in your PYTHONPATH") from e
ImportError: 'gitdb' could not be found in your PYTHONPATH
>>> quit()

I was able to resolve this by executing conda install gitdb, and was able to continue with testing.

3.

[ ] Installation: Installation succeeds as documented.

This is the same issue documented above in (2) in the package install setup.

  1. [ ] Functionality: Any functional claims of the software been confirmed.

This is the same issue documented in (1) regarding the check_mechanics evaluation.

Conceptual Suggestions

  1. It might be nice to simplify the interface to your check_mechanics function. You're clearly doing a lot of extra processing behind the scenes with your private functions to get this to work. I realize this is more work, but it would be nice to have a similar interface to the count_points function with just a filename. The function would then figure out the rest, besides the token obviously!

  2. It would be convenient if your package could include the test lab datasets. Think of how vega_datasets or dplyr has functions to generate test data withdata(mtcars). This way users could run your vignette without having to clone your repo.

  3. The code coverage at 74% is a little bit of a concern. After looking at your coverage: https://codecov.io/gh/UBC-MDS/labzen/src/main/labzen/labzen.py it looks like there were some difficulties setting up a test that would automatically perform a repo check. Was it not possible to have the check_mechanics function use the labzen repo itself? A small note is that you use the same dummy labs in each of your test functions where a single fixture input may have been ideal.

  4. When trying to debug the check_mechanics function and heading to the docs, I noticed all of the private functions you've also created. I would suggest trying to reorder the function appearance order and/or indicate with a leading '_' that these are not public functions in your package.

Minor details
  1. In your installation command you've included the terminal prompt '$' in the code chunk.
  2. Your clone instruction code chunk is missing 'git clone'
  3. The count_points vignette is missing the leading 'lz' in the check mechanics code chunk. I.e. update to: lz.count_points("data-raw/dummylab.Rmd")
  4. The contributors requirement was easy enough to satisfy by with your github contributors tab, but there was no explicit inclusion of names/emails in any repo files.
  5. The version numbering seems like a lot of updates since version 0.1.0. It certainly looks like you've included changes that would indicate a minor version update.
Summary

Again, I really liked your project idea, and I think it could be valuable to MDS students. I do wonder if the instruction team would discourage initial adoption for future cohorts as the automation would discourage pedagogical git usage. Unfortunately, I would not be able to check off all of the package submission requirements at this point. I do recognize that the scope of the project was difficult to achieve given the limited time and your group size, though!

rtaph commented 3 years ago

Dear @cmmclaug,

Thanks for taking the time to review and provide valuable feedback! We will try to provide a more thorough response/update in the coming days.

sukh2929 commented 3 years ago

Dear @cmmclaug,

Thanks for your valuable feedback! We will discuss and try to provide an update shortly.

rtaph commented 3 years ago

Dear @cmmclaug,

On behalf of the entire team, thank you for for all the feedback. We value the time you put into reviewing labzen. 🎉

Here are our line-by-line responses:

  • [ ] Vignette(s) demonstrating major functionality that runs successfully locally

A lot of this is in the README, but we'll add a short vignette to introduce this basic functionality. Expect something by close-of-business Friday.

  • [ ] Any additional setup required (authentication tokens, etc)

We are sorry to hear you were not able to get check_mechanics() to work (more on that below). We agree this could be better explained and will add a short vignette to explain how to obtain this token.

  • [ ] Installation: Installation succeeds as documented.

Thank you for reporting the installation challenges. This is something we want other users to avoid.

The error messages you shared, were those obtained after running poetry install, or pip? Did you try installing via...

### METHOD 1, pip at the command line

# first, create a fresh python environment
conda create -n labzen-test-env1 python=3 -y
conda activate labzen-test-env1

# next, install via pip
pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple labzen

... before installing via poetry. If so, did that work?

Our team tried to reproduce your error but we are not able to:

### METHOD 2, poetry at the command line. 

# first, create a fresh python environment
conda create -n labzen-test-env2 python=3 poetry -y
conda activate labzen-test-env2

# Note: make sure to be in the root of the repo.
poetry install

# open an interpreter
python

For us, this opens a python interpreter and runs normally:

Python 3.9.2 | packaged by conda-forge | (default, Feb 21 2021, 05:02:20) 
[Clang 11.0.1 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from labzen import labzen as lz
>>> df, tab = lz.count_points()
The existing files are:
1.dummylab.ipynb
2.dummylab.Rmd
Enter your file number from the above list:1
>>> tab
           type  total  prop
0  Non-Optional     19  0.95
1      Optional     20  1.00
2           All     39  1.95

Would you mind letting us know if Method 1 and Method 2 result in the same installation interruption when you run them in a fresh environment?

  • [ ] Functionality: Any functional claims of the software been confirmed.

We're sorry to hear that you were not able to get check_mechanics() to work. One thing we want to verify with you, did you obtain a token from Github Enterprise (GHE), or Github.com? Since the function checks GHE, it will not work if it is a token from Github.com.

In the mean time, your less-than-frictionless experience highlighted we can do more to make this process easier. Some ideas are to:

  1. Add a small vignette or md file to explain clear steps for obtaining the token (with screen caps);
    • Issue #58 [Target deadline: Friday, March 26, 2021, 17:00]
  2. Add a utility function that checks that the token works (that a successful connection with the API can be established)
    • Issue #59 (enhancement for future release-- not planned for end-of-week)
  3. Write a helper function get_github_enterprise_token() that opens up a browser window to GHE's token page. This would be similar to R's usethis::create_github_token() command.
    • Issue #60 (enhancement for future release-- not planned for end-of-week)

Review Comments

Hey labzen team,

Congratulations on a clever and creative project idea! With about 80 labs in the program by my estimate, this is definitely something your target audience can relate to and use! After going through the code, this was certainly an ambitious project, and I commend your hustle. I apologize for having to report any negative feedback, but hopefully I can provide another set of eyes to help polish the package!

Don't apologize! We appreciate your honest feedback and think our package will be stronger for it.

Requirements

I was unable to check off all of the package requirements as described below:

[ ] Vignette(s) demonstrating major functionality that runs successfully locally

It was not clear to us whether that was needed for the Python package assignment or not, but we will try to create a standalone intro vignette to tick this box.

I was unable to get the check_mechanics function to work as described in the vignette. I tried to execute the commands in the Python interpreter, but there is no provided token for the sample inputs. The .gif is a nice touch, but there is no mention of JupyterLab in the package and I went off script to reproduce there, but was also unsuccessful. Finally, I tried with a personal repo, but received the following error as described below:

>>> pyfile = "lab1.ipynb"
>>> reponame = "DSCI_563_lab1_cswag7"
>>> lz.check_mechanics(reponame, pyfile)

Sorry you were not able to get it to work!

What was your working directory when you ran the above? Were you in your DSCI_563_lab1 folder? Is there any chance that the token was for Github.com and not for GHE?

We are trying to think of other potential explanations but these are the first that come to mind.

[ ] Any additional setup required (authentication tokens, etc)

The first additional setup note required would be an authentication token to execute the check_mechanics function as noted above.

After package installation I ran poetry install to install any package dependencies I may have been missing. Unfortunately, I received the following the following error message:

[truncated]

  File "/opt/miniconda3/lib/python3.8/site-packages/git/__init__.py", line 27, in _init_externals
    raise ImportError("'gitdb' could not be found in your PYTHONPATH") from e
ImportError: 'gitdb' could not be found in your PYTHONPATH
>>> quit()

I was able to resolve this by executing conda install gitdb, and was able to continue with testing.

We're not really sure what the explanation is, but it may have to do with the environment in which you installed the package? None of us are Python experts, but we seem to recall that older versions of pip do not do dependency resolution well, so it might be that if you had some of the dependencies installed, that maybe the wrong package versions existed in the environment?

By running pip --version at the command line, we were all on 21.0.1.

If you do end up trying Methods 1 and Method 2 in a fresh conda environment, would you mind letting us know if you get the same error? If it is still an issue, we may need to ask an instructor/TA to advise.

Conceptual Suggestions
  1. It might be nice to simplify the interface to your check_mechanics function. You're clearly doing a lot of extra processing behind the scenes with your private functions to get this to work. I realize this is more work, but it would be nice to have a similar interface to the count_points function with just a filename. The function would then figure out the rest, besides the token obviously!

Yes, that's a great idea. In our R package version, we implemented something similar where the repo path was inferred from the path to the lab file.

  1. It would be convenient if your package could include the test lab datasets. Think of how vega_datasets or dplyr has functions to generate test data withdata(mtcars). This way users could run your vignette without having to clone your repo.

None of us have tried this before, but that is a great idea. Let us research how we can make that work so that the sample files are available even if you did pip install instead of poetry.

  1. The code coverage at 74% is a little bit of a concern. After looking at your coverage: codecov.io/gh/UBC-MDS/labzen/src/main/labzen/labzen.py it looks like there were some difficulties setting up a test that would automatically perform a repo check. Was it not possible to have the check_mechanics function use the labzen repo itself? A small note is that you use the same dummy labs in each of your test functions where a single fixture input may have been ideal.

Unfortunately, this is very difficult to test. The reason are that:

  1. When trying to debug the check_mechanics function and heading to the docs, I noticed all of the private functions you've also created. I would suggest trying to reorder the function appearance order and/or indicate with a leading '_' that these are not public functions in your package.

Great suggestion!

Minor details
  1. In your installation command you've included the terminal prompt '$' in the code chunk.

Is this not automatic from the cookiecutter template? Indicating that it's a shell command? We can easily remove it.

  1. Your clone instruction code chunk is missing 'git clone'

Great idea. We think this might best be added to the contributing section of our readme, where we can also give clearer instructions on how to install and test with poetry.

  1. The count_points vignette is missing the leading 'lz' in the check mechanics code chunk. I.e. update to: lz.count_points("data-raw/dummylab.Rmd")

Great catch! Thanks for flagging.

  1. The contributors requirement was easy enough to satisfy by with your github contributors tab, but there was no explicit inclusion of names/emails in any repo files.

Our names are listed in the README directly as well as in the license and toml files.

However, we'll add a separate file so that we can also recognize contributions from community members who are not the main authors.

Speaking of... we would love to include your name in that file! Do we have your permission to list you as a contributor? If so, what name spelling and email would you like us to use?

  1. The version numbering seems like a lot of updates since version 0.1.0. It certainly looks like you've included changes that would indicate a minor version update.

Yes, for some reason the two different Actions trigger different version bumps. The first time we ran the action, it also created a whole sequence of version commits on the runner and we're not sure why. In the future, we'll consider switching to a workflow with an intermediary dev branch. For now, since this is the workflow we know best, we'll tolerate some needless version bumps :)

Summary

Again, I really liked your project idea, and I think it could be valuable to MDS students. I do wonder if the instruction team would discourage initial adoption for future cohorts as the automation would discourage pedagogical git usage. Unfortunately, I would not be able to check off all of the package submission requirements at this point. I do recognize that the scope of the project was difficult to achieve given the limited time and your group size, though!

Thanks for taking the time to review!

Hopefully we can get to the bottom of the installation hiccups. We've listed follow-up action items for ourselves in Github issues. We hope to have the most critical elements addressed by Friday 17:00 PST.

Sincerely,

Raf, Kamal, and Sukhdeep

cmmclaug commented 3 years ago

Wow, thorough responses!

Thanks to your comprehensive investigations, I was able to resolve the following requirement hiccups as described:

As for the small details, it looks like you've got everything sorted out and covered with issues already!

Our names are listed in the README directly as well as in the license and toml files.

However, we'll add a separate file so that we can also recognize contributions from community members who are not the main authors.

Speaking of... we would love to include your name in that file! Do we have your permission to list you as a contributor? If so, what name spelling and email would you like us to use?

This one is reviewer error! Thanks for responding kindly. Sure, I can accept the role of clueless QA with Craig McLaughlin, cmmclaug@gmail.com.

I will hold off on submitting an updated package review template until after all of your efforts, but I can see you've already resolved a bunch!

kmoravej commented 3 years ago

Hi Craig,

Thank you for spending time and effort to make sure that the installation and check_mechanics works.

We will get back to you by Saturday to address your suggestions.

jufu commented 3 years ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

Readme requirements The package meets the readme requirements below:

The README should include, from top to bottom:

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider:

Functionality

For packages co-submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing: 1.5

Review Comments

Installation

There were three ways mentioned to install. The readme's version of pip install and the git clone approach worked, however the readthedocs.io version (pip install -u labzen) did not.

For reference, I'm using Linux (Ubuntu 20.04.2 LTS). I tested most of the functionality using a jupyter notebook.

Documentation

We may want to include some dedicated sections around authentication and interacting with github, but perhaps this will be in a future version of this package.

Functionality

I installed labzen and tried to follow the examples in the module documentation:

https://labzen.readthedocs.io/en/latest/source/labzen.html#module-labzen

It could be me, but I had trouble getting the token working. I tried multiple approaches, but couldn't get it to work based off of the example code in the readthedocs documentation.

Works

The following works well when pointing to the local path!

Doesn't work

First off, kudos for attempting github integration and interactive functions in Jupyter (ex. prompts for entering input after the main call). This is not easy considering our other workload and the main aspects of this project course.

check_commits(repo_name: Str)

I couldn't figure out how to get this working. I had a jupyter notebook running, set the token (need to prefix lz. with gettoken()), but when I ran the following it said the token was missing still.

I'm also not sure what the comments meant about providing a local path to the folder. I tried referencing my Linux path as well, but with no luck

from labzen import labzen as lz

token = lz.gettoken()
lz.check_commits("DSCI_563_lab1_jufu")

#also tried this
lz.check_commits("/home/jufu/UBC_MDS/courses/block05/DSCI_563_lab1_jufu")
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
<ipython-input-11-cc9e281d9a32> in <module>
----> 1 lz.check_commits("DSCI_563_lab1_jufu")

~/miniconda3/lib/python3.8/site-packages/labzen/labzen.py in check_commits(repo_name)

NameError: name 'token' is not defined

check_lat_version(repo_name: str)[source]


lz.check_lat_version("DSCI_563_lab1_jufu")

#also tried this
token = lz.gettoken()
lz.check_lat_version("DSCI_563_lab1_jufu")

I got a little bit further here, but ended up with this error after entering my token twice (I was prompted twice)

---------------------------------------------------------------------------
BadCredentialsException                   Traceback (most recent call last)
<ipython-input-7-83a378376484> in <module>
      1 token = lz.gettoken()
----> 2 lz.check_lat_version("DSCI_599_lab1_jene3456")

~/miniconda3/lib/python3.8/site-packages/labzen/labzen.py in check_lat_version(repo_name)
    278     token = gettoken()
    279     g = Github(token, base_url="https://github.ubc.ca/api/v3")
--> 280     org = g.get_organization("MDS-2020-21")
    281 
    282     # get the repo name and and the last commit from the remote

~/miniconda3/lib/python3.8/site-packages/github/MainClass.py in get_organization(self, login)
    309         """
    310         assert isinstance(login, str), login
--> 311         headers, data = self.__requester.requestJsonAndCheck("GET", "/orgs/" + login)
    312         return github.Organization.Organization(
    313             self.__requester, headers, data, completed=True

~/miniconda3/lib/python3.8/site-packages/github/Requester.py in requestJsonAndCheck(self, verb, url, parameters, headers, input)
    313 
    314     def requestJsonAndCheck(self, verb, url, parameters=None, headers=None, input=None):
--> 315         return self.__check(
    316             *self.requestJson(
    317                 verb, url, parameters, headers, input, self.__customConnection(url)

~/miniconda3/lib/python3.8/site-packages/github/Requester.py in __check(self, status, responseHeaders, output)
    338         output = self.__structuredFromJson(output)
    339         if status >= 400:
--> 340             raise self.__createException(status, responseHeaders, output)
    341         return responseHeaders, output
    342 

BadCredentialsException: 401 {"message": "Bad credentials", "documentation_url": "https://docs.github.com/enterprise/2.22/rest"}

check_mechanics(repo_name: str, file_name: Optional[str] = None)[source]

from labzen import labzen as lz
file = "dummylab.ipynb"
lz.check_mechanics("labzen", file)

Encountered the same issue with BadCredentialsException.

There are two repo_name parameters in the documentation. Encountered the same issue regarding BadCredentialsException after entering token.

Additionaly, please review your example code in the docstrings as I tried to quickly copy and paste this code to try out, but had to edit them due to some incorrect references.

Automated Tests

I think the requirements were met as we had to write automated tests for 3 of the functions. However, I think for this first version of the package perhaps we should have excluded some of the functions that weren't working yet. Specifically the ones that was using the token. In hindsight, I understand the difficulties in doing this within the time constraints we had for this course.

Continuous Integration

This looks like it is functioning based on the Actions tab history.

Summary

This was a valiant effort to try to tackle a project like this. There were many challenges rannging from the github integration including token authentication, to parsing multiple file types that could contain almost anything just due to what jupyter and Rmd workbooks are.

The base functions that you focused on worked with my basic testing and there's promise to where you can build on from this start. The base functions I'm referring to are:

Automating more of this workflow would be lovely for both the students, TAs and instructors as I'm sure this takes quite a bit of time for everyone. Overall, great job in building this in such a short amount of time.

kmoravej commented 3 years ago

Dear Justin,

Thank you for taking the time to review our package and provide your valuable feedback! We will discuss and try to provide a more thorough response in the coming days.

cmmclaug commented 3 years ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

It is understood that reviewers and authors are all members of the same UBC MDS 2021 cohort.

Documentation

The package includes all the following forms of documentation:

Readme requirements The package meets the readme requirements below:

The README should include, from top to bottom:

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider:

Functionality

For packages co-submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing: 4

Review Comments

Hey labzen team,

Here is the updated package review template I promised! I can see everyone has been working hard on improving your package and it's great to see. I can see PRs still coming in late last night, but I am also aware of all of the improvements made to address the core issues.

Requirements
Conceptual Suggestions

1. It might be nice to simplify the interface to your check_mechanics function. You're clearly doing a lot of extra processing behind the scenes with your private functions to get this to work. I realize this is more work, but it would be nice to have a similar interface to the count_points function with just a filename. The function would then figure out the rest, besides the token obviously! Issue addressed here: https://github.com/UBC-MDS/labzen/issues/61

  1. It would be convenient if your package could include the test lab datasets. Think of how vega_datasets or dplyr has functions to generate test data withdata(mtcars). This way users could run your vignette without having to clone your repo. Issue addressed here, easier to access lab datasets: https://github.com/UBC-MDS/labzen/issues/62

  2. The code coverage at 74% is a little bit of a concern. After looking at your coverage: https://codecov.io/gh/UBC-MDS/labzen/src/main/labzen/labzen.py it looks like there were some difficulties setting up a test that would automatically perform a repo check. Was it not possible to have the check_mechanics function use the labzen repo itself? A small note is that you use the same dummy labs in each of your test functions where a single fixture input may have been ideal.

  3. When trying to debug the check_mechanics function and heading to the docs, I noticed all of the private functions you've also created. I would suggest trying to reorder the function appearance order and/or indicate with a leading '_' that these are not public functions in your package. Issue addressed here: https://github.com/UBC-MDS/labzen/issues/63

Minor details

1.~~ In your installation command you've included the terminal prompt '$' in the code chunk.~~

  1. Your clone instruction code chunk is missing 'git clone'
  2. The count_points vignette is missing the leading 'lz' in the check mechanics code chunk. I.e. update to: lz.count_points("data-raw/dummylab.Rmd")
  3. The contributors requirement was easy enough to satisfy by with your github contributors tab, but there was no explicit inclusion of names/emails in any repo files.
  4. The version numbering seems like a lot of updates since version 0.1.0. It certainly looks like you've included changes that would indicate a minor version update.
Summary

Great work addressing review comments and continuing to expand your package functionality!

rtaph commented 3 years ago

Dear @jufu,

Thanks so much for taking the time to review! Here are some inline responses:

  • [ ] Vignette(s) demonstrating major functionality that runs successfully locally

We have updated our readthedocs documentation. There should now be

Installation

There were three ways mentioned to install. The readme's version of pip install and the git clone approach worked, however the readthedocs.io version (pip install -u labzen) did not.

Thank you bringing that oversight to our attention. We've fixed it here

For reference, I'm using Linux (Ubuntu 20.04.2 LTS). I tested most of the functionality using a jupyter notebook.

Thanks for letting us know! We did not have any Linux users in our team so it is great that you were able to test it out on this OS.

Documentation

We may want to include some dedicated sections around authentication and interacting with github, but perhaps this will be in a future version of this package.

Done via PR 68. We now have a vignette for Github tokens on readthedocs.

Functionality

I installed labzen and tried to follow the examples in the module documentation:

labzen.readthedocs.io/en/latest/source/labzen.html#module-labzen

It could be me, but I had trouble getting the token working. I tried multiple approaches, but couldn't get it to work based off of the example code in the readthedocs documentation.

Sorry to hear that!

Kamal, Sukhdeep and I overhauled check_mechanics() to make it more straightforward to use 🌟🌟🌟 . These are breaking changes 💥 to the API. The syntax now is:

from labzen import labzen as lz
lz.check_mechanics(
    path = "~/Dropbox/mds/b5/563_ul/lab2", 
    token = "b46b7b41fab337d24d1a43f5e1e"
)

The syntax also can use your working directory, so that if you are in an assignment repo, you can more simply run:

from labzen import labzen as lz
lz.check_mechanics(
    token = "b46b7b41fab337d24d1a43f5e1e"
)

We also added a function to make generating a token much easier, by opening up a browser window to Github Enterprise 🔐:

from labzen import labzen as lz
lz.create_github_token()

For this version of the package, we did not bother using a credentials manager, so you'll need to pass-in the token as a parameter each time.

check_commits(repo_name: Str)

I couldn't figure out how to get this working. I had a jupyter notebook running, set the token (need to prefix lz. with gettoken()), but when I ran the following it said the token was missing still.

Thanks for raising this-- we've scaffolded check_commits(), check_lat_version(), and check_repo_link() so that they are now private functions called by check_mechanics(). If you manage to run check_mechanics() smoothly, then that means check_commits() works!

Additionally, please review your example code in the docstrings as I tried to quickly copy and paste this code to try out, but had to edit them due to some incorrect references.

We agree, these examples were not very straightforward and reproducible.

We made the examples in our Usage Vignette and module docstrings much clearer and easier to reproduce.

Automated Tests

I think the requirements were met as we had to write automated tests for 3 of the functions. However, I think for this first version of the package perhaps we should have excluded some of the functions that weren't working yet. Specifically the ones that was using the token. In hindsight, I understand the difficulties in doing this within the time constraints we had for this course.

For Milestone 2, we clarified to the TAs (via Canvas) that our "third" function would be check_commits(), not check_mechanics(), check_lat_version(), or check_repo_link(). As we were very pressed for time that week (hackathon), we only focused on the minimum requirements for the Milestone. However, adding more tests in the future would definitely be a priority.

Overall, great job in building this in such a short amount of time.

Thanks for taking the time to review our 📦 ! The three of us have learned some new tricks in the process of implementing your suggestions !

With your permission, we would like to acknowledge your reviewing efforts in our contributors.md file. May we put your name and email, and if so, how would you like it to show?

Best,

Raf, Sukhdeep, and Kamal

rtaph commented 3 years ago

Dear @cmmclaug ,

Thanks for having gone the extra mile to re-review our changes 🕺🏻.

We've addressed the terminal $ issue here.

It's been a long journey refactoring our package and we're glad we did it with all your inputs! But our energy stores for writing more unit tests are pretty depleted 💀. We'll keep that one parked... for now 🅿️.