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

Crowd-Kit Learning #47

Merged
merged 13 commits into from Nov 18, 2022
Merged

Crowd-Kit Learning #47

merged 13 commits into from Nov 18, 2022

Conversation

pilot7747
Copy link
Contributor

This is just an example of what this subpackage will contain.

We need to configure setup.cfg and add new tests. Here I suggest to discuss the concept.

@pilot7747
Copy link
Contributor Author

Received a concept approve by @dustalov. Now start to adding tests and trying to configure it as a subpackage with extra requirements. Also waiting for approve by @zdchu to use a code derived from the original CoNAL implementation

@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2022

Codecov Report

Base: 93.02% // Head: 93.07% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (7a1f876) compared to base (3129208).
Patch coverage: 98.05% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
+ Coverage   93.02%   93.07%   +0.04%     
==========================================
  Files          43       47       +4     
  Lines        1908     2007      +99     
==========================================
+ Hits         1775     1868      +93     
- Misses        133      139       +6     
Impacted Files Coverage Δ
crowdkit/aggregation/__init__.py 66.66% <ø> (-27.46%) ⬇️
crowdkit/learning/crowd_layer.py 94.73% <94.73%> (ø)
crowdkit/learning/__init__.py 100.00% <100.00%> (ø)
crowdkit/learning/conal.py 100.00% <100.00%> (ø)
crowdkit/learning/text_summarization.py 52.38% <100.00%> (ø)
crowdkit/learning/utils.py 100.00% <100.00%> (ø)

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pilot7747
Copy link
Contributor Author

pilot7747 commented Nov 17, 2022

@dustalov The PR is ready for a review. @zdchu has approved the code usage and now the tests are passing.

@pilot7747 pilot7747 changed the title Initial commit for Crowd-Kit Learning Crowd-Kit Learning Nov 17, 2022
@dustalov
Copy link
Collaborator

  • Please add py.typed to every new source directory, excluding tests: https://github.com/Toloka/crowd-kit/blob/main/crowdkit/py.typed
  • Kindly move the summarization aggregation tests to the learning module tests
  • We have to do it in a separate PR, but have you tried installing Crowd-Kit without the learning module to a bare venv? Has it worked?
  • If you initialize parameters randomly, is it possible to choose the seed value and then canonize the tests?

@pilot7747
Copy link
Contributor Author

pilot7747 commented Nov 18, 2022

  • Kindly move the summarization aggregation tests to the learning module tests

These tests have never existed. One of the reasons it is difficult to implement them is the fact they are extremely heavy. You need to download a 3 GB model from HuggingFace and do this on every commit.

  • If you initialize parameters randomly, is it possible to choose the seed value and then canonize the tests?

Yes, but I'm not sure this will be consistent across platforms and PyTorch versions.

@dustalov
Copy link
Collaborator

Good. We're almost there. Please fix the newlines and we're good to go.

@pilot7747
Copy link
Contributor Author

Please fix the newlines and we're good to go.

What newlines?

@pilot7747
Copy link
Contributor Author

This one: https://github.com/Toloka/crowd-kit/pull/47/files/b4d07cc2eb30db31ca6234d8bec4f7530afe3dec#diff-f1e0b25eeb099b940ecba4b0d893c38ef7ad7d122ab962a58118fef33936d4f5

Done. I specifically checked for other py.typed files and looked if they have a trailing newline but I didn't realize that GitHub doesn't show trailing newlines.

Copy link
Collaborator

@dustalov dustalov left a comment

Choose a reason for hiding this comment

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

Good.

crowdkit/learning/py.typed Outdated Show resolved Hide resolved
@dustalov dustalov merged commit 0ada334 into main Nov 18, 2022
@pilot7747
Copy link
Contributor Author

Btw, yes, I've checked, the package installs without learning extra

@dustalov dustalov deleted the learning-init branch March 22, 2023 15:45
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.

None yet

3 participants