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

CI: Removed visualize-annotation-stats visualizer because of compatibility issues with altair and pyarrow #24

Merged
merged 15 commits into from
Jan 17, 2024

Conversation

VinzentRisch
Copy link
Collaborator

@VinzentRisch VinzentRisch commented Jan 8, 2024

This PR solves #34

  • Moved tests out of q2_amr/test directory into q2_amr/card directory to agree with lab standards.
  • Added the right path to test data in setup.py.
  • Removed visualize-annotations-stats visualizer and all associated functions from reads.py
  • Removed everything associated with visualize-annotations-stats from plugin_setup.py and README.md.
  • Removed the altair package from meta.yaml

Now the pyarrow dependency is not a problem anymore because altair was removed.

@misialq
Copy link
Contributor

misialq commented Jan 12, 2024

Hey @VinzentRisch, I think there is only one solution to move this forward (at least for now). Since we cannot solve the environment to make it work, maybe we could (after all) simply remove this visualization action (despite of what we just discussed recently) - it's not really useful right now and it introduces the dependency on Altair. If you are ok with this, I would suggest you to:

  • make a copy of this branch with a clear name and push it to the upstream - just to have a copy in case we want to bring this functionality back at a later stage
  • remove all the code related to the visualizer
  • update the README
  • remove the Altair dependency

Does this make sense? Am I missing something?

@VinzentRisch
Copy link
Collaborator Author

VinzentRisch commented Jan 16, 2024

Hi @misialq, I dind't find ay other solution.
But with the visualize-annotation-stats removed it seems to work.
This is now all in one PR even though it should be at least two, but i cant merge them independently because otherwise the CI fails.
So should i leave it like this or do two separate ones, and for the one where i remove altair and the action you have to lift the restrictions and I can merge it with a failing CI?

Copy link
Contributor

@misialq misialq left a comment

Choose a reason for hiding this comment

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

Hey @VinzentRisch, I think we can leave it as is - thanks! See the small cosmetic suggestion below + please update it with the changes you merged from the other PR (the black step is not working). After that, feel free to merge 🙌

setup.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3f01352) 66.58% compared to head (f096db0) 93.35%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #24       +/-   ##
===========================================
+ Coverage   66.58%   93.35%   +26.77%     
===========================================
  Files          13       18        +5     
  Lines         832     1054      +222     
===========================================
+ Hits          554      984      +430     
+ Misses        278       70      -208     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@VinzentRisch VinzentRisch linked an issue Jan 17, 2024 that may be closed by this pull request
@VinzentRisch VinzentRisch changed the title TEST: Moved tests out of test directory to card directory CI: Removed visualize-annotation-stats visualizer because of compatibility issues with altair and pyarrow Jan 17, 2024
@VinzentRisch VinzentRisch merged commit 68f5ddb into bokulich-lab:main Jan 17, 2024
6 checks passed
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.

Tests don't run in CI, because of pyarrow dependency of Altair
3 participants