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

Global seed changes #146

Closed
wants to merge 11 commits into from
Closed

Conversation

SirRob1997
Copy link
Contributor

@SirRob1997 SirRob1997 commented Jul 20, 2021

This PR is for the changes discussed in #145. I thought removing it from the interface made the most sense, so we don't pass any redundant information and only set the seed for both random and numpy once in initialize.py. I've also used this to fix some remaining spacy.load issues in some of the filters/transformations so they are loaded only once in initialize.py.

Would be nice if you could test all things to make sure I didn't break anything.

Draft for now, since I think some tests need to be adjusted now since they used a different seed.

@SirRob1997
Copy link
Contributor Author

SirRob1997 commented Jul 20, 2021

@kaustubhdhole & @aadesh11 can you take a look at this? random and numpy should now have a global seed of 0 and these should be shared by default across imports. I'm not exactly sure why the butter_fingers_perturbation results in a different output, since it was using 0 as a seed before as well. Nevertheless, some transformations were using seed 42 and the corresponding tests would most definitely need to be adjusted which I don't want to do without explicit approval. This change would also need to be propagated to the evaluation leaderboard to reflect the alternated seed and since we're changing the interface, new PR's should reflect that change as well.

@kaustubhdhole
Copy link
Collaborator

Hi @SirRob1997, this looks very promising! Yes, it is okay to change the values of the test cases to reflect the changes off a new seed! I would be happy to re-test it myself too if you need a final confirmation. :)

@aadesh11
Copy link
Collaborator

Thanks, @SirRob1997 for making the codebase looks cleaner. Please help to make the required changes in the test case of Butter Finger perturbation to pass the GitHub action checks.

@SirRob1997
Copy link
Contributor Author

Yup will adapt things either on Sunday or Monday, depending on when I can scrap a few minutes.

@SirRob1997
Copy link
Contributor Author

SirRob1997 commented Jul 25, 2021

EDIT: Fixed it, there was something missing in requirements.txt

I'm having trouble getting one of the transformations to run, so it's hard for me to adjust this last test, all others pass now. Does someone know what I need to do to resolve these issues:

sentence_reordering:

allennlp.common.checks.ConfigurationError: coref not in acceptable choices for dataset_reader.type: ['babi', 'conll2003', 'interleaving', 'multitask', 'multitask_shim', 'sequence_tagging', 'sharded', 'text_classification_json']. You should either use the --include-package flag to make sure the correct module is loaded, or use a fully qualified class name in your config file like {"model": "my_module.models.MyModel"} to have it imported automatically.

@SirRob1997 SirRob1997 marked this pull request as ready for review July 25, 2021 21:45
@SirRob1997
Copy link
Contributor Author

SirRob1997 commented Jul 25, 2021

The pytest Github action is failing because it evaluates against the already existing tests and not my changes to them. I don't think there's anything I can do on my end to fix that, right @kaustubhdhole @aadesh11? My changes result in reproducible results on local and pass all tests, can you guys verify?

@kaustubhdhole
Copy link
Collaborator

For Review @aadesh11 @AbinayaM02

Copy link
Collaborator

@AbinayaM02 AbinayaM02 left a comment

Choose a reason for hiding this comment

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

Fix for pytest failure.

@AbinayaM02 AbinayaM02 self-requested a review July 30, 2021 13:53
Copy link
Collaborator

@AbinayaM02 AbinayaM02 left a comment

Choose a reason for hiding this comment

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

Please resolve the conflicts and let's see if the pytest goes through without any issue.

@kaustubhdhole
Copy link
Collaborator

@SirRob1997 - thank you very much for your changes - can you please resolve the conflicts

@SirRob1997
Copy link
Contributor Author

Hey, sorry I'm really busy recently and unfortunately I don't think I'll find time over the next few weeks to fix these issues. Hopefully someone else can implement the idea...

@SirRob1997 SirRob1997 closed this Aug 26, 2021
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.

4 participants