Skip to content

fix: add assets and use local file links to allow offline reports#2904

Merged
johanneskoester merged 27 commits intosnakemake:mainfrom
fs82:offline-reporting
Aug 16, 2024
Merged

fix: add assets and use local file links to allow offline reports#2904
johanneskoester merged 27 commits intosnakemake:mainfrom
fs82:offline-reporting

Conversation

@fs82
Copy link
Copy Markdown
Contributor

@fs82 fs82 commented May 31, 2024

This PR solves the issue with the offline reports. Several files are downloaded on the fly while generating the report(js, css and licenses). They are now added to the package as assets.

I used the test_report test case to verify that the generated report is the same and works as expected. One minor change in the report appears, as I thought it is more logical to use major.minor version numbering for the react package as well.

To my opinion a update of the documentation is not necessary.

Closes #282

QC

  • The PR contains a test case for the changes or the changes are already covered by an existing test case.
  • The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

Summary by CodeRabbit

  • New Features

    • Improved installation process with the addition of setuptools-download for enhanced dependency management.
    • New section added to manage the downloading of essential resources during installation.
  • Bug Fixes

    • Enhanced file path handling to support both absolute and relative paths, ensuring better file accessibility.
  • Chores

    • Reaffirmed inclusion of the LICENSE.md file in the package distribution for compliance.
  • Refactor

    • Updated package resource management to utilize local paths, improving reliability and portability.
    • Streamlined HTML template for loading JavaScript resources, enhancing performance and maintainability.

@fs82 fs82 requested a review from johanneskoester as a code owner May 31, 2024 13:38
@fs82 fs82 changed the title Add assets and use local file links fix: add assets and use local file links to allow offline reports May 31, 2024
@fs82
Copy link
Copy Markdown
Contributor Author

fs82 commented May 31, 2024

@johanneskoester The checks are failing on the new files added to the repository as assets for the reports. To my opinion they should not be checked by the ci of snakemake, as they are obtained from other projects. However, my knowledge about Github and its CI is limited. Would be good if you can comment on how to work around this.

@fs82
Copy link
Copy Markdown
Contributor Author

fs82 commented May 31, 2024

The only other way I see is to compress the files and extract the on the fly?

@johanneskoester
Copy link
Copy Markdown
Contributor

I think the files can simply be excluded via adding a sonar-project.properties file to the repo: https://docs.sonarsource.com/sonarcloud/advanced-setup/analysis-scope/#file-exclusion-and-inclusion

@fs82
Copy link
Copy Markdown
Contributor Author

fs82 commented Jun 4, 2024

I think the files can simply be excluded via adding a sonar-project.properties file to the repo: https://docs.sonarsource.com/sonarcloud/advanced-setup/analysis-scope/#file-exclusion-and-inclusion

Ok, sounds good. I will have a look asap.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jun 4, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
5 Security Hotspots
C Reliability Rating on New Code (required ≥ A)
D Security Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@fs82
Copy link
Copy Markdown
Contributor Author

fs82 commented Jun 4, 2024

I think the files can simply be excluded via adding a sonar-project.properties file to the repo: https://docs.sonarsource.com/sonarcloud/advanced-setup/analysis-scope/#file-exclusion-and-inclusion

I give up, is there a way to see somehow the log output of the sonar cloud? I tried several ways to ignore the assets folder, but it does not seem to be that the sonar cloud respects the settings in sonar-project.properties. It is honestly not my field of knowledge 😄

@daaaaande
Copy link
Copy Markdown

would be happy to see that feature. using snakemake on a cluster that is offline, and the reports could help. is there maybe a workaround for this issue?

@rahedges
Copy link
Copy Markdown

I would also make use of this feature, offline reports are necessary for my snakemake analysis.

@fs82
Copy link
Copy Markdown
Contributor Author

fs82 commented Jun 19, 2024

I think the files can simply be excluded via adding a sonar-project.properties file to the repo: https://docs.sonarsource.com/sonarcloud/advanced-setup/analysis-scope/#file-exclusion-and-inclusion

I give up, is there a way to see somehow the log output of the sonar cloud? I tried several ways to ignore the assets folder, but it does not seem to be that the sonar cloud respects the settings in sonar-project.properties. It is honestly not my field of knowledge 😄

Ok from cross reading the sonar cloud manual, it looks like there are automatic analysis enabled for snakemake repository, due to the presence of the .sonarcloud.properties file. This seems to conflict with the sonar-project.properties file, which is only for ci-based analysis. I honestly dont get the difference, but the manual states, this is conflicting. I move the exclude settings now to the .sonarcloud.properties, lets see what happens

https://docs.sonarsource.com/sonarcloud/advanced-setup/automatic-analysis/#presence-of-a-properties-file

@johanneskoester
Copy link
Copy Markdown
Contributor

Actually, I think we can solve the issue in a way that fixes two problems at a time.
It is in general not so desirable to have copies of foreign code within the repo.
So why don't we make their download part of the package building process, e.g. via setuptools-download? This way, they won't be checked by any linter, and the source tree and git history remains clean of foreign code. Further, an update is just doable by changing the URLs in the setup.cfg. Alternatively, if more complex logic is needed, one can also put some download code using requests into the setup.py.

@fs82
Copy link
Copy Markdown
Contributor Author

fs82 commented Jun 21, 2024

Actually, I think we can solve the issue in a way that fixes two problems at a time. It is in general not so desirable to have copies of foreign code within the repo. So why don't we make their download part of the package building process, e.g. via setuptools-download? This way, they won't be checked by any linter, and the source tree and git history remains clean of foreign code. Further, an update is just doable by changing the URLs in the setup.cfg. Alternatively, if more complex logic is needed, one can also put some download code using requests into the setup.py.

Ok, the latest commit tries to apply your suggestion. I tried to make the get_resource_as_a_string function backward compatible. The download during package build seems to work for all License, css and js files, except for tailwindcss. The content delivery network (CDN) of tailwindcss does not allow access via urllib.request (used by setuptools-download). The reason is probably a wrong header, but this can only be solved in setuptools-download to my understanding. Anyhow, when flying over the tailwindcss website, this it seems to be not a good idea to use in production the Play CDN. They strongly recommend to build a css file for the project locally and supply it.

@johanneskoester Sorry to say, but this is all I can do. I have no clue what tailwind does, how it is used and in particular not how it is integrated in snakemake.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Aug 16, 2024

Caution

Review failed

The pull request is closed.

Walkthrough

The recent changes primarily enhance the Snakemake report generation process for offline environments by improving the handling of local resources. Key modifications include the addition of local paths for license files and assets, ensuring that the reports do not rely on external CDNs. This enhances usability for users operating without internet access, aligning with the objective of generating reports solely from local resources.

Changes

File Path Change Summary
MANIFEST.in Reasserted inclusion of LICENSE.md, added newline.
setup.cfg Added setup_requires for setuptools-download and new section for managing local resource downloads.
snakemake/report/html_reporter/common.py Updated get_resource_as_string to handle both absolute and relative paths.
snakemake/report/html_reporter/data/packages.py Modified get_packages to use local paths for licensing information instead of hardcoded URLs.
snakemake/report/html_reporter/template/index.html.jinja2 Changed JavaScript resource references to use local paths directly.

Assessment against linked issues

Objective Addressed Explanation
Allow offline report generation by using local resources instead of CDN (##282)
Enable passing custom CSS paths to the report command (##282) Custom CSS paths functionality not implemented.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between 6bd67d7 and 025a0c6.

Files selected for processing (4)
  • MANIFEST.in (1 hunks)
  • setup.cfg (2 hunks)
  • snakemake/report/html_reporter/common.py (1 hunks)
  • snakemake/report/html_reporter/data/packages.py (2 hunks)
Additional context used
Path-based instructions (2)
snakemake/report/html_reporter/common.py (1)

Pattern **/*.py: Do not suggest to add trailing commas.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.

snakemake/report/html_reporter/data/packages.py (1)

Pattern **/*.py: Do not suggest to add trailing commas.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.

Ruff
snakemake/report/html_reporter/common.py

17-17: Use context handler for opening files

(SIM115)

snakemake/report/html_reporter/data/packages.py

29-29: Trailing comma missing

Add trailing comma

(COM812)

Additional comments not posted (5)
MANIFEST.in (1)

3-3: Reaffirmed inclusion of LICENSE.md.

The inclusion of LICENSE.md in the manifest file ensures that the license is part of the distribution, which is essential for compliance and transparency.

snakemake/report/html_reporter/data/packages.py (1)

Line range hint 5-53: Enhanced portability with local paths.

The use of sys.prefix to construct local paths for package resources improves portability and robustness across different environments, aligning with the PR's objectives to support offline report generation.

Tools
Ruff

29-29: Trailing comma missing

Add trailing comma

(COM812)


55-55: Trailing comma missing

Add trailing comma

(COM812)

setup.cfg (3)

53-53: Addition of smart-open dependency looks good.

The addition of smart-open >=4.0,<8.0 is consistent with the versioning strategy used for other dependencies.


66-66: Inclusion of setuptools-download is appropriate.

The addition of setuptools-download >= 1.1.0 ensures that necessary tools for downloading data files are available during the setup process.


68-117: New [setuptools_download] section is well-structured.

The section provides a systematic approach to managing external resources with URLs and SHA256 checksums for integrity verification.

To ensure correctness, verify that all URLs are accessible and checksums match the files.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between 025a0c6 and 949536b.

Files selected for processing (1)
  • snakemake/report/html_reporter/common.py (1 hunks)
Additional context used
Path-based instructions (1)
snakemake/report/html_reporter/common.py (1)

Pattern **/*.py: Do not suggest to add trailing commas.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.

Additional comments not posted (1)
snakemake/report/html_reporter/common.py (1)

13-18: Use a context manager for file operations.

To ensure proper resource management and avoid potential file handle leaks, use a context manager when opening files. This was flagged in previous reviews.

-        with open(fpath) as file:
-            return file.read()
+        with open(fpath) as file:
+            return file.read()

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between 949536b and e61c021.

Files selected for processing (1)
  • snakemake/report/html_reporter/data/packages.py (2 hunks)
Additional context used
Path-based instructions (1)
snakemake/report/html_reporter/data/packages.py (1)

Pattern **/*.py: Do not suggest to add trailing commas.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.

Additional comments not posted (2)
snakemake/report/html_reporter/data/packages.py (2)

5-6: Imports are appropriate.

The addition of sys and os imports is necessary for constructing local paths for resources.


21-53: Local path construction is correct.

The use of os.path.join(sys.prefix, ...) to construct local paths for package resources is a robust solution for offline environments.

Ensure that the local paths used here exist and are correctly set up during installation or packaging.

@johanneskoester johanneskoester enabled auto-merge (squash) August 16, 2024 15:49
@sonarqubecloud
Copy link
Copy Markdown

@johanneskoester johanneskoester merged commit 9cd94f7 into snakemake:main Aug 16, 2024
@johanneskoester
Copy link
Copy Markdown
Contributor

Oh, that is clearly a bug with automerge. It merged before all the tests were done, and some even failed!!

@johanneskoester
Copy link
Copy Markdown
Contributor

I will check the main branch and see if I can fix it in a quick follow-up.

johanneskoester added a commit that referenced this pull request Aug 16, 2024
johanneskoester pushed a commit that referenced this pull request Aug 19, 2024
🤖 I have created a release *beep* *boop*
---


##
[8.18.1](v8.18.0...v8.18.1)
(2024-08-19)


### Bug Fixes

* add assets and use local file links to allow offline reports
([#2904](#2904))
([9cd94f7](9cd94f7))
* use query from storage object in order to be able to reflect possible
modifications (via StorageProvider.postprocess_query())
([#3031](#3031))
([3ddae58](3ddae58))


### Documentation

* clarify config file location
([6bd67d7](6bd67d7))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
johanneskoester added a commit that referenced this pull request Aug 28, 2024
…me possible offline (cont. of #2904) (#3026)

The original PR has been reverted in main because it does not seem to
work will with developer installation mode (assets are not installed in
the right place in that case).

cc @fs82 

### QC
<!-- Make sure that you can tick the boxes below. -->

* [x] The PR contains a test case for the changes or the changes are
already covered by an existing test case.
* [x] The documentation (`docs/`) is updated to reflect the changes or
this is not necessary (e.g. if the change does neither modify the
language nor the behavior or functionalities of Snakemake).


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Summary by CodeRabbit

- **New Features**
- Introduced structured asset management within the Snakemake framework,
allowing for better handling and deployment of external resources.

- **Improvements**
- Enhanced package metadata handling for improved maintainability and
accessibility of license and source files.
- Enabled the inclusion of additional package data during installation,
ensuring necessary files are available.

- **Bug Fixes**
- Improved reliability by transitioning to local paths for resource
referencing, enhancing accessibility and reducing dependency on external
URLs.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
johanneskoester pushed a commit that referenced this pull request Aug 29, 2024
🤖 I have created a release *beep* *boop*
---


##
[8.19.0](v8.18.2...v8.19.0)
(2024-08-29)


### Features

* check consistency of output file mtimes (must be newer than input
files) ([#3050](#3050))
([666cf62](666cf62))
* print host name when executing workflow
([#3048](#3048))
([b0ff787](b0ff787))


### Bug Fixes

* `mem` and `disk` inference fixes
([#3040](#3040))
([7530794](7530794))
* avoid error accessing superclass in ioutils
([#3056](#3056))
([a66a5f5](a66a5f5))
* disable execute after print compilation
([#3041](#3041))
([86ed3cd](86ed3cd))
* download report assets upon package build such that reports become
possible offline (cont. of
[#2904](#2904))
([#3026](#3026))
([e8dad4b](e8dad4b))


### Documentation

* Add 'Editor integrations' section to Installation page
([#3045](#3045))
([9a4006d](9a4006d))
* Fix typo (seesee to see)
([#3037](#3037))
([de201fb](de201fb))
* various documentation fixes
([#3052](#3052))
([b11460c](b11460c))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
vandalt pushed a commit to vandalt/snakemake that referenced this pull request Aug 29, 2024
…me possible offline (cont. of snakemake#2904) (snakemake#3026)

The original PR has been reverted in main because it does not seem to
work will with developer installation mode (assets are not installed in
the right place in that case).

cc @fs82 

### QC
<!-- Make sure that you can tick the boxes below. -->

* [x] The PR contains a test case for the changes or the changes are
already covered by an existing test case.
* [x] The documentation (`docs/`) is updated to reflect the changes or
this is not necessary (e.g. if the change does neither modify the
language nor the behavior or functionalities of Snakemake).


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Summary by CodeRabbit

- **New Features**
- Introduced structured asset management within the Snakemake framework,
allowing for better handling and deployment of external resources.

- **Improvements**
- Enhanced package metadata handling for improved maintainability and
accessibility of license and source files.
- Enabled the inclusion of additional package data during installation,
ensuring necessary files are available.

- **Bug Fixes**
- Improved reliability by transitioning to local paths for resource
referencing, enhancing accessibility and reducing dependency on external
URLs.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
vandalt pushed a commit to vandalt/snakemake that referenced this pull request Aug 29, 2024
🤖 I have created a release *beep* *boop*
---


##
[8.19.0](snakemake/snakemake@v8.18.2...v8.19.0)
(2024-08-29)


### Features

* check consistency of output file mtimes (must be newer than input
files) ([snakemake#3050](snakemake#3050))
([666cf62](snakemake@666cf62))
* print host name when executing workflow
([snakemake#3048](snakemake#3048))
([b0ff787](snakemake@b0ff787))


### Bug Fixes

* `mem` and `disk` inference fixes
([snakemake#3040](snakemake#3040))
([7530794](snakemake@7530794))
* avoid error accessing superclass in ioutils
([snakemake#3056](snakemake#3056))
([a66a5f5](snakemake@a66a5f5))
* disable execute after print compilation
([snakemake#3041](snakemake#3041))
([86ed3cd](snakemake@86ed3cd))
* download report assets upon package build such that reports become
possible offline (cont. of
[snakemake#2904](snakemake#2904))
([snakemake#3026](snakemake#3026))
([e8dad4b](snakemake@e8dad4b))


### Documentation

* Add 'Editor integrations' section to Installation page
([snakemake#3045](snakemake#3045))
([9a4006d](snakemake@9a4006d))
* Fix typo (seesee to see)
([snakemake#3037](snakemake#3037))
([de201fb](snakemake@de201fb))
* various documentation fixes
([snakemake#3052](snakemake#3052))
([b11460c](snakemake@b11460c))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

offline snakemake --report

4 participants