Metadata intake by lucasortizny · Pull Request #87 · facebookresearch/aepsych

lucasortizny · 2022-07-08T02:28:34Z

Here is my pull request for the metadata intake. Some notes to recap:

The implementation in server.py is in handle_setup_v01. As requested, handle_setup has been left intact.
The master table has two new columns: participant_id (unique, nullable, max size is 50 characters in length, auto-generated UUID4 if not specified) and extra_metadata (4096 characters in length).
The extra metadata information will be serialized/deserialized -> JSON and placed into the Database in that way for future reference.
The Database schematic I originally created will need to be updated with the new information.

Please let me know if there are any questions! This should help cover #32

…cs for if a change is detected

…p to handle in server.py

…sych into metadata-intake

facebook-github-bot · 2022-07-08T02:28:38Z

Hi @lucasortizny!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2022-07-08T04:15:42Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

facebook-github-bot · 2022-07-08T04:58:10Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

mshvartsman · 2022-07-08T17:10:12Z

                ),
                **kwargs,
            )
+    # Convert config into a dictionary (but as it is, excludes "common")


Is that right? The way ConfigParser works, things that are in common are mirrored to all other sections. What this method is doing (I think) is omitting these mirrored copies (basically if something is in common, and also shows up in another section, just ignore the one in the other section since it's a mirror).

Oh I absolutely did not know that. I knew default sections in ConfigParsers were special but now it makes sense why this line is here!

Ok -- can you update the comment to be correct or remove?

Yeah I'll update it, haven't pushed any commits yet

mshvartsman · 2022-07-08T17:12:11Z

        return None

-    def record_setup(self, description, name, id=None, request=None) -> str:
+    def record_setup(self, description, name, extrametadata=None, id=None, request=None, participant_id=None) -> str:


can you underscore extra_metadata?

mshvartsman · 2022-07-08T17:46:51Z

+            ### if the metadata does not exist, we are going to log nothing
+            else:
+                self._db_master_record = self.db.record_setup(
+                    description="default description",


shouldn't this be experiment_name and experiment_description instead of these hardcoded defaults?

Which line(s) in particular? @mshvartsman is it the else condition where string values are given instead?

Yeah I'm not sure what I was thinking. If there's no metadata we can keep it to some default string.

mshvartsman · 2022-07-08T17:49:57Z

-                id=experiment_id,
-            )
+            ### make a temporary config object to derive parameters because server handles config after table
+            tempconfig = Config(**request["message"])


Why not modify self.configure to take in a Config object instead of the raw message? Then you don't need to create it twice (the first thing self.configure does is create a Config anyway).

Hey @mshvartsman ! I absolutely agree in that it would be a great idea to just pass the config object. One thing I am concerned about is that self.configure is also called by the legacy handle_setup function with a string. I would have to write another version of this function to adjust to the new handle_setup_v01 but again it is important to think about whether you'd like to see another secondary version of an existing function. Please let me know if you'd like me to proceed. (TLDR: I don't want to modify the original handle_setup in any way unless absolutely necessary).

I think modifying the original handle_setup is maybe okay here. To the extent that there's a contract here it's that legacy handle_setup's behavior doesn't change, which will still be true if you change where the string-to-config conversion happens. I don't think anything else is calling self.configure. @crasanders what do you think?

Yeah, I think it's fine to change handle_setup to pass a Config into self.configure.

Okie dokie, will do!

mshvartsman · 2022-07-08T17:51:07Z

-
+            
            self._db_master_record = self.db.record_setup(
-                description="default description",


I don't think these are particularly necessary capitalization / whitespace changes

I don't have an opinion one way or the other about capitalization, but do make sure that it's consistent. It might be useful to define DEFAULT_NAME and DEFAULT_DESC constants.

mshvartsman · 2022-07-08T18:11:35Z

Great start -- left some nits in the code. Can you put together a minimal example that constructs some messages with metadata, processes them through versioned_handler, and then gets that metadata back out? That would be good as a test, and also as an example for users.

Also, once #89 lands you should run ufmt on your changes.

crasanders · 2022-07-12T16:30:16Z

    experiment_name = Column(String(256))
    experiment_description = Column(String(2048))
    experiment_id = Column(String(10), unique=True)
+    participant_id = Column(String(50), unique=True, nullable=True)


Do we want participant_id to be nullable?

I originally wanted it that way but then I eventually wrote it so a UUID would be generated anyways so this line no longer makes sense so I will remove the nullability!

crasanders · 2022-07-12T16:41:56Z

-                id=experiment_id,
-            )
+            ### make a temporary config object to derive parameters because server handles config after table
+            tempconfig = Config(**request["message"])


Yeah, I think it's fine to change handle_setup to pass a Config into self.configure.

crasanders · 2022-07-12T16:50:31Z

-
+            
            self._db_master_record = self.db.record_setup(
-                description="default description",


I don't have an opinion one way or the other about capitalization, but do make sure that it's consistent. It might be useful to define DEFAULT_NAME and DEFAULT_DESC constants.

… remove nullability, modified both handle_setup's for new configure command, changed comment to take into account actual behavior

…sych into metadata-intake

…ests.

lucasortizny · 2022-07-14T21:30:49Z

Attached is the current Database schematic updated to this PR's changes. I am going to continue this PR by writing unit tests for the new functionality in the DB.

…sych into metadata-intake

…nerated on setup

…e will see

:

This reverts commit 3e6615f.

lucasortizny · 2022-08-18T18:44:42Z

The reason why config was changed to usedconfig is because the metadata-intake functionality introduces a "config" parameter in the configure function in line 677 that I don't want to run into variable naming issues.

facebook-github-bot · 2022-08-18T20:10:10Z

@crasanders has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Here is my pull request for the metadata intake. Some notes to recap: - The implementation in server.py is in handle_setup_v01. As requested, handle_setup has been left intact. - The master table has two new columns: participant_id (unique, nullable, max size is 50 characters in length, auto-generated UUID4 if not specified) and extra_metadata (4096 characters in length). - The extra metadata information will be serialized/deserialized -> JSON and placed into the Database in that way for future reference. - The Database schematic I originally created will need to be updated with the new information. Please let me know if there are any questions! This should help cover facebookresearch#32 Pull Request resolved: facebookresearch#87 Differential Revision: D38840357 Pulled By: crasanders fbshipit-source-id: 3549bdf887df4f09829cc9088d3a303b29932dfa

Summary: Here is my pull request for the metadata intake. Some notes to recap: - The implementation in server.py is in handle_setup_v01. As requested, handle_setup has been left intact. - The master table has two new columns: participant_id (unique, nullable, max size is 50 characters in length, auto-generated UUID4 if not specified) and extra_metadata (4096 characters in length). - The extra metadata information will be serialized/deserialized -> JSON and placed into the Database in that way for future reference. - The Database schematic I originally created will need to be updated with the new information. Please let me know if there are any questions! This should help cover facebookresearch#32 Pull Request resolved: facebookresearch#87 Differential Revision: D38840357 Pulled By: crasanders fbshipit-source-id: e5e6ce31bca2e959349204afaec99e5d6e6270b1

Summary: Here is my pull request for the metadata intake. Some notes to recap: - The implementation in server.py is in handle_setup_v01. As requested, handle_setup has been left intact. - The master table has two new columns: participant_id (unique, nullable, max size is 50 characters in length, auto-generated UUID4 if not specified) and extra_metadata (4096 characters in length). - The extra metadata information will be serialized/deserialized -> JSON and placed into the Database in that way for future reference. - The Database schematic I originally created will need to be updated with the new information. Please let me know if there are any questions! This should help cover facebookresearch#32 Pull Request resolved: facebookresearch#87 Differential Revision: D38840357 Pulled By: crasanders fbshipit-source-id: 6f07bd944e4184f5e951383f28ded5fff4850e42

Summary: Here is my pull request for the metadata intake. Some notes to recap: - The implementation in server.py is in handle_setup_v01. As requested, handle_setup has been left intact. - The master table has two new columns: participant_id (unique, nullable, max size is 50 characters in length, auto-generated UUID4 if not specified) and extra_metadata (4096 characters in length). - The extra metadata information will be serialized/deserialized -> JSON and placed into the Database in that way for future reference. - The Database schematic I originally created will need to be updated with the new information. Please let me know if there are any questions! This should help cover facebookresearch#32 Pull Request resolved: facebookresearch#87 Test Plan: New unit tests Differential Revision: https://internalfb.com/D38840357 Pulled By: crasanders fbshipit-source-id: 328b4cbc6ccd03a1e9869c70dbf18f875846327d

lucasortizny added 5 commits July 6, 2022 19:57

Modified configuration file in order to be serializable from/to JSON

8e3d2a1

Modified table components to allow for new columns and update mechani…

0b7cb94

…cs for if a change is detected

Added default options for new parameters in the database for the setu…

e213130

…p to handle in server.py

Modified server.py's v01 setup command to handle the new parameters

284084c

Merge branch 'metadata-intake' of https://github.com/lucasortizny/aep…

1367d51

…sych into metadata-intake

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 8, 2022

mshvartsman reviewed Jul 8, 2022

View reviewed changes

crasanders reviewed Jul 12, 2022

View reviewed changes

lucasortizny added 8 commits July 12, 2022 16:04

Merge branch 'facebookresearch:main' into metadata-intake

0f6f54d

Modified configure command to accept object, modified table master to…

99cc2de

… remove nullability, modified both handle_setup's for new configure command, changed comment to take into account actual behavior

Merge branch 'metadata-intake' of https://github.com/lucasortizny/aep…

3b9209d

…sych into metadata-intake

Formatted using ufmt

4cc5cb8

Modified config to permit old functionality for the sake of old unitt…

fd6e484

…ests.

Attempt to fix boolean errors in configure function

bb45a88

Handled the double asterisk operator for config_args

b3ec5c3

Run ufmt on modified changes

521f87e

lucasortizny added 5 commits July 15, 2022 18:08

Created my first little baby unit test awwwww

ff5a12b

Merge branch 'metadata-intake' of https://github.com/lucasortizny/aep…

8a0cb92

…sych into metadata-intake

Ran ufmt on the changes

3d5379d

Merge branch 'facebookresearch:main' into metadata-intake

d8fd5bd

Merge branch 'facebookresearch:main' into metadata-intake

3e2c2c3

lucasortizny added 16 commits August 3, 2022 19:32

Created some unit tests for individual field testing

80bcc32

Hopefully fixed some of the dictionary referencing

7c5d94f

Added some more tests between the json and the master_table record ge…

2d46a87

…nerated on setup

Fixed some of the testing...

c347655

Added some screwed up db testing

d22523f

Added some malformed metadata, not sure if the error test works but w…

6b961ff

…e will see

Hopefully fixes the alt issue

c02bce4

Test if the server modified if statements will cause problesm

b9b7b07

:

Adding this commit so I can access from another location

c3a9013

Reverted the config test

3e6615f

Removed the redundant functionality

6416a31

Revert "Reverted the config test"

79206b6

This reverts commit 3e6615f.

Created configuration json test

1077bd7

Added sample configuration json for testing purposes

296da11

Ran ufmt

79f8abf

Trying to fix

c5c94d8

lucasortizny requested review from crasanders and mshvartsman August 11, 2022 23:40

lucasortizny added 2 commits August 18, 2022 14:22

Adjusted properly

646b3e8

Fixed a merge conflict from recent config changes

ddf2bbc

facebook-github-bot closed this in 213907b Aug 29, 2022

mshvartsman mentioned this pull request Sep 23, 2022

Write documentation and tutorials for the database #37

Closed

mshvartsman mentioned this pull request Oct 14, 2022

Build a metadata intake flow #32

Closed



		self._db_master_record = self.db.record_setup(
		description="default description",

Conversation

lucasortizny commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jul 8, 2022

Action Required

Process

Uh oh!

facebook-github-bot commented Jul 8, 2022

Uh oh!

facebook-github-bot commented Jul 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshvartsman Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasortizny Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshvartsman commented Jul 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasortizny commented Jul 14, 2022

Uh oh!

lucasortizny commented Aug 18, 2022

Uh oh!

facebook-github-bot commented Aug 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lucasortizny commented Jul 8, 2022 •

edited

Loading

mshvartsman Jul 12, 2022 •

edited

Loading

lucasortizny Jul 8, 2022 •

edited

Loading