[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

Fix report crash by freezing SQLAlchemy and doclinks fix #233

Merged
merged 6 commits into from
Jul 8, 2021
Merged

Fix report crash by freezing SQLAlchemy and doclinks fix #233

merged 6 commits into from
Jul 8, 2021

Conversation

northwestwitch
Copy link
Member
@northwestwitch northwestwitch commented Jul 8, 2021 •
edited
Loading

This PR adds | fixes:

How to prepare for test:

How to test:

  • Switch to this branch and install again chanjo (pip install -e .)
  • Repeat the procedure described in the issue and notice that the html report is now working

Expected outcome:

  • That it works!

Review:

  • Code approved by
  • Tests executed by CR

This version is a:

  • MAJOR - when you make incompatible API changes
  • MINOR - when you add functionality in a backwards compatible manner
  • PATCH - when you make backwards compatible bug fixes or documentation/instructions

@northwestwitch northwestwitch changed the title update version Fix report crash by freezing SQLAlchemy Jul 8, 2021
northwestwitch
northwestwitch commented Jul 8, 2021
View reviewed changes
requirements.txt
@@ -6,3 +6,4 @@ toolz
ruamel.yaml
importlib_metadata
pymysql
sqlalchemy==1.3.*
Copy link
Member Author

Choose a reason for hiding this comment

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

@mhkc this is the real fix to the report crash problem!

northwestwitch
northwestwitch commented Jul 8, 2021
View reviewed changes
setup.py Outdated
version='4.6.0',

description='Coverage analysis tool for clinical sequencing',
version="4.6.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only line I have changed in this file.

@northwestwitch
Copy link
Member Author

@moonso are you working by chance?

@northwestwitch northwestwitch added dependencies Pull requests that update a dependency file priority urgent labels Jul 8, 2021
@northwestwitch northwestwitch changed the title Fix report crash by freezing SQLAlchemy Fix report crash by freezing SQLAlchemy and doclinks Jul 8, 2021
@northwestwitch northwestwitch changed the title Fix report crash by freezing SQLAlchemy and doclinks Fix report crash by freezing SQLAlchemy and doclinks fix Jul 8, 2021
@northwestwitch
Copy link
Member Author

Behavior installing master branch on a fresh conda env and launching the report: ⛔

image

image

Behavior installing this branch: 🆗

image

image

@mikaell mikaell self-requested a review July 8, 2021 07:18
mikaell
mikaell approved these changes Jul 8, 2021
View reviewed changes
Copy link
@mikaell mikaell left a comment

Choose a reason for hiding this comment

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

Looks good. (Maybe we should freeze every dependency some day -but I suppose that's gonna happen automatically in Docker installations).

setup.py Outdated
sys.exit()


def parse_reqs(req_path='./requirements.txt'):
def parse_reqs(req_path="./requirements.txt"):
"""Recursively parse requirements from nested pip files."""
Copy link

Choose a reason for hiding this comment

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

Bonus points for recursion 😃

@mhkc
Copy link
mhkc commented Jul 8, 2021

Its working with the chanjo-report container as well.

@northwestwitch
Copy link
Member Author

Thanks @mhkc and @mikaell, I'm going to merge and release then! 🥳

@mhkc
Copy link
mhkc commented Jul 8, 2021

Looks good. (Maybe we should freeze every dependency some day -but I suppose that's gonna happen automatically in Docker installations).

Yes but every time you build a new version of the container you pull the latest version of some packages.

@northwestwitch northwestwitch merged commit a542376 into master Jul 8, 2021
@northwestwitch northwestwitch deleted the freeze_mysqlalchemy branch July 8, 2021 07:29
@northwestwitch northwestwitch mentioned this pull request Jul 8, 2021
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikaell mikaell mikaell approved these changes

@moonso moonso Awaiting requested review from moonso

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file priority urgent
Projects
None yet
Milestone
No milestone
3 participants
@northwestwitch @mhkc @mikaell