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

Documents #32

Merged
merged 3 commits into from
Sep 7, 2021
Merged

Documents #32

merged 3 commits into from
Sep 7, 2021

Conversation

mramputatoes
Copy link

@mramputatoes mramputatoes commented Sep 6, 2021

Description

Updated README.md to include how the run the DS API locally when using a Mac OS with the Apple Silicone aram (M1 processor). https://www.youtube.com/watch?v=NJ6t97MmWiA

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • This change requires a documentation update

Change status

  • Complete, tested, ready to review and merge
  • Complete, but not tested (may need new tests)
  • Incomplete/work-in-progress, PR is for discussion/feedback

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Previewed on GitHub, looks good.

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

  • I have pulled from master before submitting this PR
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My code has been reviewed by at least one peer (the peer review to approve a PR counts. The reviewer must download and test the code)\
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • There are no merge conflicts

Copy link
Contributor

@santorap santorap left a comment

Choose a reason for hiding this comment

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

Great job on figuring out a solution to those issues you were having with your M1 Mac, Jeremiah! I have a couple of suggestions for improvements in my comments below. I think it makes sense for them to be incorporated before merging into main, but it is up to you.

README.md Outdated
- run: `conda activate NAME`: activates the conda environment.
- The conda environment needs to running in order for the application to run.
- run: `conda install -c conda-forge -y psycopg2 numpy pandas`: install necessary dependencies.
- Create the .env file in the folder.
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of repeating the instructions from above, it makes more sense to reference them directly here. This way if there needs to be a change it only has to happen in once in one place, reducing the possibility for errors in the future.

@@ -73,6 +73,25 @@ database.
- run:`pipenv shell` to start the pipenv environment.
- run:`uvicorn app.main:app --reload` to start running the fast api.

### Running the DS application with Apple M1

Copy link
Contributor

@santorap santorap Sep 6, 2021

Choose a reason for hiding this comment

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

Maybe add a brief description of the problem you ran into that requires your to do these additional steps before starting with line 68? (I.e. Getting errors when trying to install psycopg2.)

Copy link
Contributor

@santorap santorap left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes! The instructions are much easier to follow now. Removing those duplicate steps make a big difference!

@@ -75,6 +75,7 @@ database.

### Running the DS application with Apple M1

- When you `pipenv install --dev` on the M1 you will most likely run into issues where the Pipfile will fail to lock due to issues with psycopg2. Psyocopg2 specifically has issues pip installing on the M1. After figuring out past issues with the M1, this is a work around until there is further bug fixing on compatibility.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@collinjensen collinjensen left a comment

Choose a reason for hiding this comment

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

This is a great addition to the documentation. Future M1 users can now get up to speed even quicker!

@@ -73,6 +73,20 @@ database.
- run:`pipenv shell` to start the pipenv environment.
- run:`uvicorn app.main:app --reload` to start running the fast api.

### Running the DS application with Apple M1

- When you `pipenv install --dev` on the M1 you will most likely run into issues where the Pipfile will fail to lock due to issues with psycopg2. Psyocopg2 specifically has issues pip installing on the M1. After figuring out past issues with the M1, this is a work around until there is further bug fixing on compatibility.
Copy link
Contributor

Choose a reason for hiding this comment

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

Very helpful to explain the issue here.

- run: `conda activate NAME`: activates the conda environment.
- The conda environment needs to running in order for the application to run.
- run: `conda install -c conda-forge -y psycopg2 numpy pandas`: install necessary dependencies.
- Create the .env file in the folder and continue following the instructions in the section above.
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually don't like reading instructions then scrolling back up to continue but I can't imagine a better place for this so... no worries

Copy link
Contributor

@ebnersam ebnersam left a comment

Choose a reason for hiding this comment

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

I agree with the nuances Paul and Collin suggested, but other than that its super good to see that you solved your problem and potentially others' problems too.

@ashtilawat23 ashtilawat23 merged commit ebf344d into main Sep 7, 2021
@ashtilawat23 ashtilawat23 deleted the Documents branch September 7, 2021 15:52
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.

5 participants