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

Consolidate logic to wait for dask futures. #109

Merged
merged 1 commit into from
Jul 31, 2023
Merged

Conversation

delucchi-cmu
Copy link
Contributor

Change Description

Reducing code duplication for waiting on dask futures, updating a tqdm task bar, dask task log, raises error, and marks stage done. Also switches from explicit file names to single stage name.

Code duplication was raised as a pylint error in the SOAP PR, so I'm trying to get ahead of it.

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (2802d8c) to head (23981df).
Report is 207 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #109   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines          881       869   -12     
=========================================
- Hits           881       869   -12     

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

Copy link

@drewoldag drewoldag left a comment

Choose a reason for hiding this comment

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

This looks pretty cool. My only question here is the same as the one I posted in slack - I'm curious at what point you would make the decision to move away from dataclass to a traditional class this work.

src/hipscat_import/catalog/run_import.py Show resolved Hide resolved
@delucchi-cmu
Copy link
Contributor Author

I hadn't been thinking too much about it, but now that you're asking me, I guess I will:

I'm in favor of keeping things plain-old-dataclasses for now, and for the foreseeable future. In nearly all cases, the methods that are being added are not modifying the state of the member variables, but are rather helper methods that could live just about anywhere. For the sake of code organization, it makes sense for them to live as class methods.

And I think to your question on slack

does the reduction of boilerplate that dataclass provides imply that maybe more of our traditional classes should be dataclasses

I think that's probably true, though there are certainly limitations and many things that will need to stay full-fledged classes. I have some biases that are showing (coming from years in java persistence layers, and years with protocol buffers) that make me lean toward these kind of container objects.

@delucchi-cmu delucchi-cmu merged commit fb9f0d0 into main Jul 31, 2023
@delucchi-cmu delucchi-cmu deleted the delucchi/prefactor branch August 7, 2023 20:06
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.

2 participants