https://outerbounds.com/ logo
Join Slack
Powered by
Hello Team, I've reviewed <Contributing doc >and n...
# dev-metaflow
s
Hello Team, I've reviewed Contributing doc and now looking for a good first issue, but the list seems old. Is the list still a good place to look for issues to get started with contribution? Appreciate your help.
ā¤ļø 1
v
hi Tushar! meow wave it's great to hear that you'd like to contribute! šŸ¤— is there some particular area / topic that you'd be interested in?
šŸ™ 1
s
I don't have any preference. I got started reviewing the codebase today. I can dig deeper in relevant module and pick issues you suggest as a good starter.
v
here's a fun one: Metaflow uses 
pylint
to check the code before execution but 
pylint
is slow compared to modern alternatives like 
flake8
and especially 
ruff
here's where `pylint` gets called - you could investigate how we could add an alternative code path for 
ruff
(e.g. if 
import ruff
works)
s
That's perfect. Thanks. I will get started.
šŸ™Œ 1
v
awesome! please follow up here when you have any early results or questions! it's preferable to share PRs early for feedback
āœ… 1
f
@victorious-lawyer-58417, I do have a Draft PR around this issue. My approach is: ā€¢ to mitigate the tightly coupled linter naming away from 
pylint
(or 
ruff
) to a generic one. ā€¢ keep 
pylint
here to stay, add capability to run lints using 
ruff
along. ā€¢ D.R.Y/unification around linting capabilities. Ofcourse, I need to work the entire implementation out and thereafter vet the functionalities(for both linters), on i- but intent for a draft PR was to vouch for the approach. NOTE: I am somehow unable to add @victorious-lawyer-58417 @salmon-shampoo-9140 (and possibly others, like Savin from the community) as 'reviewers' of this PR.
s
@flat-hospital-6180 I liked your approach to design an interface to support multiple linters, however I am not sure if we'd want to support/keep pylint around if ruff is a better choice. I will let @straight-shampoo-11124 comment on this one. Otherwise, directionally I like that it makes code extensible to include another linter in future (not a common scenario though). And, thanks for the PR! Unfortunately I haven't made progress beyond reviewing overall codebase and advocating internal team at work to adopt Metaflow. :)
šŸ™ 1
f
Thanks for your valuable review and inputs @salmon-shampoo-9140- indeed, a review from @victorious-lawyer-58417 on the approach and (concept to)implementation(thus far šŸ™‚ ) would be very helpful in setting the direction, and I'd be happy to complete the development at the earliest!
šŸ‘ 1
@victorious-lawyer-58417, it'd be super helpful to have our direction above, run by you, could you please vet the above please? I shall quickly work and test the PR out, thereafter. Thanks šŸ™‚
s
i am happy to follow up directly on the PR!
f
Thanks Savin! Let me try to squeeze out time on revisiting and completing the needful for this contribution- I totally eye a final PR this week, shall nevertheless keep you posted over the draft PR afloat already.
@ancient-application-36103, I have tagged you against a bunch of comments I'd want to run by you/team- also the PR, is almost "ready for review", largely awaiting your feedback/review/take, esp w.r.t: ā€¢ do we remove 
pylint
, altogether? this involves deleting/muting the existing capabilities and modifying contributed configurations to focus solely on ruff. ā€¢ concept to configuration/settings for ruff i.e 
ruff.toml
esp. from client/user/project p.o.v/settings. ā€¢ concept to testing the linting capabilities using ruff, introduced in this PR, esp. for running usecases/modes outside of 'local', i.e on airflow, on argo, on SFs etc. trivia: as a fun part, I have my own builds (for this PR) failing on account of the ruff runs(checks only) I (thought I'd also to the entire repo linting) introduced for the metaflow source code too, as a github workflow(Action, see: lint_repo.yml) šŸ™‚
2 Views
Open in Slack
PreviousNext
Powered by