Skip to content

Conversation

@enryH
Copy link
Collaborator

@enryH enryH commented Dec 20, 2024

Make report generator executable from anywhere.

@enryH
Copy link
Collaborator Author

enryH commented Dec 20, 2024

I added integration tests for all scripts. I still need to allow the report to be called as vuegen directly (without the python -m call). Looking at the test, I see an error for jupyter which somehow does not lead to the execution failing:

https://github.com/Multiomics-Analytics-Group/vuegen/actions/runs/12429117006/job/34701961965

ERROR: Validation of YAML front matter failed.
ERROR: In file quarto_report/quarto_report.qmd
(line 9, columns 5--19) Field "format" has value kernel: python3, which must instead be no possible value
 8:   jupyter:
 9:     kernel: python3
       ~~~~~~~~~~~~~~~
10: ---
ERROR: Render failed due to invalid YAML.

Is that okay? Do you want to do something about it? If it is an actual error, I would argue that the execution stops and return a non-zero exit code.

@enryH enryH requested a review from sayalaruano December 20, 2024 13:19
@enryH
Copy link
Collaborator Author

enryH commented Dec 20, 2024

@sayalaruano I think its's ready also without the jupyter error. If I read it correctly it's mainly about the default kernel. maybe it Python3 on the runner. Could be tested in a codespace...

@sayalaruano
Copy link
Collaborator

sayalaruano commented Dec 20, 2024

Hey @enryH! I took a look at the jupyter error, and got the same result in my local env. It seems that another way to create the ipynb file from the qmd file is with the command:

quarto convert basics-jupyter.qmd

https://quarto.org/docs/tools/jupyter-lab.html

I can add this change in the quarto_reportview.py file. Do you think this will be better, or should we just keep as it is?

@enryH
Copy link
Collaborator Author

enryH commented Dec 20, 2024

Does the error have an impact or not? I think we can discuss this once we tested it a bit... But if you know what should be the result, and the changed command does it, go ahead:)

@enryH enryH linked an issue Dec 20, 2024 that may be closed by this pull request
5 tasks

```bash
python vuegen/main.py --config docs/example_data/MicW2Graph/report_config_micw2graph.yaml --report_type streamlit
cd docs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey! Why is it necessary to change the directory to docs? In my local environment, it's working when I execute the program from the base folder, not from docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have to check when I am back, but you could double check in a clean environment within a code space on that branch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, I will check that out. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just tested in a code space clean environment and it works without changing the directory to docs. Also, I identified some errors with the docx, odt, and pdf formats when converting a df into an image:
OSError: Chrome executable not able to be found on your machine

Copy link
Collaborator Author

@enryH enryH Jan 13, 2025

Choose a reason for hiding this comment

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

Well. Then this is another requirement. So which tool relies on chronium? Probably something for the interactive dataframes (itables)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why cd docs:

  1. I wanted to check that the import is not local. vuegen is on the main folder, not jet using the src layout, i.e. moving the packages into a separate src folder.
  2. For building the website I will need to have the example report relative to the docs/conf.py file. In the end someone who uses the report generator will have only a folder resembling the docs.

In short: yes we can remove the docs, but let's to it in a separate PR where we change the layout of the repo.

Copy link
Collaborator

@sayalaruano sayalaruano Jan 13, 2025

Choose a reason for hiding this comment

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

I think that dataframe_image is the one that relies on chromium.
It's ok to remove the cd docs in an independent PR. And should we merge the current PR into the meain with the cd docs?

Copy link
Collaborator Author

@enryH enryH Jan 14, 2025

Choose a reason for hiding this comment

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

I will do it now.

@enryH enryH merged commit 3d029fc into main Jan 14, 2025
10 checks passed
@enryH enryH deleted the entrypoint branch January 14, 2025 15:17
@enryH enryH linked an issue Jan 30, 2025 that may be closed by this pull request
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.

ToDos Make the package pip installable

3 participants