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

Upload pdf to s3 #38

Closed
wants to merge 9 commits into from
Closed

Upload pdf to s3 #38

wants to merge 9 commits into from

Conversation

Cannon07
Copy link
Collaborator

@Cannon07 Cannon07 commented Apr 29, 2024

Title: Automated Book build and Upload to S3.

Changes Made:

  • Updated build_pdf_book.py
  • Added Unit Test script: test_build_pdf_book.py for testing the build_pdf_book.py script.
  • Added Mock files in the mocks directory for running the test_build_pdf_book.py script.
  • Updated the test.yml workflow to run the test_build_pdf_book.py script.
  • Added build_pdf_book.yml workflow to fetch the latest version of tutorials, rebuild the book and upload to S3 bucket

Copy link
Contributor

@GreatRSingh GreatRSingh left a comment

Choose a reason for hiding this comment

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

LGTM, few minor fixes and good to merge.

if __name__ == "__main__":
os.system("mkdir " + PDF_PATH)
html_to_pdf()
merge_pdf()
compile_information_pages()
merge_pdf_pages(['storage/title.pdf', 'storage/acknowledgement.pdf', 'storage/contents.pdf', 'storage/full_pdf.pdf'])
merge_pdf_pages(['storage/title.pdf', 'storage/acknowledgement.pdf', 'storage/contents.pdf', 'storage/merged.pdf'])
upload_file('storage/full_pdf.pdf', 'deepchemtutorials', 'TutorialsBook.pdf')
Copy link
Contributor

Choose a reason for hiding this comment

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

pls, keep an empy line at the end



if __name__ == "__main__":
unittest.main()
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, pls add an empty last line.

@@ -5,6 +5,7 @@
- pdfunite
- pdfkit
- mdpdf
- boto3
Copy link
Contributor

Choose a reason for hiding this comment

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

please add boto3 to requirements file

@@ -0,0 +1,42 @@
name: Build latest version of PDF Book

on:
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, can you please explain when exactly this workflow will get triggered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the workflow will be triggered whenever new commits are performed in examples/tutorials directory in deepchem repository.

import json


class TestConvertHTMLToPDF(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Add a docstring to this class (use numpydoc style)


class TestConvertHTMLToPDF(unittest.TestCase):

def test_convert_html_to_pdf(self):
Copy link
Member

Choose a reason for hiding this comment

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

Add docstrings to methods

os.rmdir("mocks/mock_temp_storage")


class TestMergePDF(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Docs for this class

Copy link
Member

@rbharath rbharath left a comment

Choose a reason for hiding this comment

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

Can you please update the README? Make suer to explain how these new capabilities work. Also please add docstrings in numpydocs style .

Why do we need to add mock-html notebooks? We didn't need these previously

@Cannon07 Cannon07 closed this May 4, 2024
@Cannon07 Cannon07 reopened this May 4, 2024
@Cannon07
Copy link
Collaborator Author

Cannon07 commented May 4, 2024

Why do we need to add mock-html notebooks? We didn't need these previously

Since the function html_to_pdf converts the html files to pdf, we need some mock html files to unit test this functionality. So for testing that I have added mock-html notebooks folder which contains sample html-files.
We didn't need it previously because there were no tests to evaluate this functionality.

@Cannon07 Cannon07 closed this May 22, 2024
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.

3 participants