fix: add assets and use local file links to allow offline reports#2904
fix: add assets and use local file links to allow offline reports#2904johanneskoester merged 27 commits intosnakemake:mainfrom
Conversation
|
@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. |
|
The only other way I see is to compress the files and extract the on the fly? |
|
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. |
|
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 |
|
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? |
|
I would also make use of this feature, offline reports are necessary for my snakemake analysis. |
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 |
|
Actually, I think we can solve the issue in a way that fixes two problems at a time. |
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. |
|
Caution Review failedThe pull request is closed. WalkthroughThe 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
Assessment against linked issues
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 theselfargument of methods.
Do not suggest type annotation of theclsargument 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 theselfargument of methods.
Do not suggest type annotation of theclsargument 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.mdin 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.prefixto 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 ofsmart-opendependency looks good.The addition of
smart-open >=4.0,<8.0is consistent with the versioning strategy used for other dependencies.
66-66: Inclusion ofsetuptools-downloadis appropriate.The addition of
setuptools-download >= 1.1.0ensures 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>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 theselfargument of methods.
Do not suggest type annotation of theclsargument 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()
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Additional comments not posted (2)
snakemake/report/html_reporter/data/packages.py (2)
5-6: Imports are appropriate.The addition of
sysandosimports 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.
|
|
Oh, that is clearly a bug with automerge. It merged before all the tests were done, and some even failed!! |
|
I will check the main branch and see if I can fix it in a quick follow-up. |
🤖 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>
…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>
🤖 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>
…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>
🤖 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>







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_reporttest 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 thereactpackage as well.To my opinion a update of the documentation is not necessary.
Closes #282
QC
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
setuptools-downloadfor enhanced dependency management.Bug Fixes
Chores
LICENSE.mdfile in the package distribution for compliance.Refactor