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

Minor Documentation Fixes: TaskID for Example Custom Flow; Comment on Homepage; More documentation for components #1243

Merged
merged 8 commits into from
Apr 18, 2023

Conversation

LennartPurucker
Copy link
Contributor

@LennartPurucker LennartPurucker commented Apr 15, 2023

Closes #1241

Moreover, we might want to add or rework the process for the examples if the IDs can change, maybe in relation to #1227.

Also Closes #1229

Also CLoses related to #1231 if we think this documentation suffices for the use case.

@LennartPurucker LennartPurucker changed the title Fix TaskID for Example Custom Flow Minor Documenation FIxes: TaskID for Example Custom Flow; Comment on Homepage Apr 15, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -7.75 ⚠️

Comparison is base (bb3793d) 85.24% compared to head (da9f1b1) 77.49%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1243      +/-   ##
===========================================
- Coverage    85.24%   77.49%   -7.75%     
===========================================
  Files           38       38              
  Lines         5008     5008              
===========================================
- Hits          4269     3881     -388     
- Misses         739     1127     +388     
Impacted Files Coverage Δ
openml/utils.py 91.25% <100.00%> (ø)

... and 15 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@LennartPurucker LennartPurucker changed the title Minor Documenation FIxes: TaskID for Example Custom Flow; Comment on Homepage Minor Documentation Fixes: TaskID for Example Custom Flow; Comment on Homepage Apr 16, 2023
@LennartPurucker LennartPurucker changed the title Minor Documentation Fixes: TaskID for Example Custom Flow; Comment on Homepage Minor Documentation Fixes: TaskID for Example Custom Flow; Comment on Homepage; More documentation for componets Apr 16, 2023
@LennartPurucker LennartPurucker changed the title Minor Documentation Fixes: TaskID for Example Custom Flow; Comment on Homepage; More documentation for componets Minor Documentation Fixes: TaskID for Example Custom Flow; Comment on Homepage; More documentation for components Apr 16, 2023
@LennartPurucker
Copy link
Contributor Author

Patch coverage has no change and project coverage change: -0.10 ⚠️

We could add tests that verify that our examples are not crashing.
I am thinking about very simple tests like:

def test_run_custom_flow_example():
    example_code = __import__("examples.30_extended.custom_flow_")

This would increase the code coverage and would allow us to see if our examples are actually working.
However, I am not sure if this is really good practice (but I also have no better idea right now).
I would be fine with it, but I feel like this depends on your preferences.

@LennartPurucker
Copy link
Contributor Author

We might want to connect this to #1070 and resolve both at the same time.

@mfeurer
Copy link
Collaborator

mfeurer commented Apr 17, 2023

I agree that this closes #1241 and #1229. I am not sure if it documents #1231 sufficiently.

RE testing the examples: the examples are executed by the workflows that build and deploy the documentation. Therefore, we automatically check them, but they do not contribute to the test coverage.

RE #1070 I don't think we can resolve this as we also look into tasks, and they cannot be addressed by names.

Copy link
Collaborator

@mfeurer mfeurer left a comment

Choose a reason for hiding this comment

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

Happy to merge if/when unit tests pass.

@LennartPurucker LennartPurucker removed the request for review from PGijsbers April 17, 2023 09:21
@LennartPurucker
Copy link
Contributor Author

LennartPurucker commented Apr 17, 2023

@mfeurer the usual tests seem to pass. Does that suffice for you? IMO, I want to have the docs build working.

If I have the time this week, I will try to fix all failing tutorials (while building the docs) and include these fixes in the PR. I think this would be appropriate with the current idea of the PR.

@LennartPurucker
Copy link
Contributor Author

@mfeurer I think this is ready for merging now. Or do you require any other changes?

@mfeurer mfeurer merged commit fb9f9eb into develop Apr 18, 2023
@mfeurer mfeurer deleted the minor_examples_update branch April 18, 2023 13:17
github-actions bot pushed a commit that referenced this pull request Apr 18, 2023
…m Flow; Comment on Homepage; More documentation for `components` (#1243)
@PGijsbers
Copy link
Collaborator

Examples are tested, I don't really see a point for calculating coverage metrics on the example code itself. As far as I am aware, this is also not something we do. The difference in code coverage is more likely to be because of changes to the test server state (and thus different error/code paths). If there are parts of the code only covered by examples and not unit tests, then the right way to correct that would be to add unit tests.

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.

Wrong references in 'Creating Custom Flow' tutorial Comment in main example is wrong
4 participants