HumanDynamics / openPDS

openpds.media.mit.edu
MIT License
110 stars 33 forks source link

coupling with funf #38

Open dcalacci opened 9 years ago

dcalacci commented 9 years ago

Hey, new to the project, been cleaning up the code and migrating the codebase to django 1.7 and other stuff like that on my own fork.

I'm wondering why some modules - AccessControlledInternalDataStore in particular - are coupled so tightly with Funf. For example, that class has a mapping field that directly relates to funf data:

        self.mapping = {'ActivityProbe': 'activity_probe',
                        'SmsProbe': 'sms_probe',
                        'CallLogProbe': 'call_log_probe',
                        'BluetoothProbe': 'bluetooth_probe',
                        'WifiProbe': 'wifi_probe',
                        'LocationProbe': 'simple_location_probe',
                        'ScreenProbe': 'screen_probe',
                        'RunningApplicationsProbe': 'running_applications_probe',
                        'HardwareInfoProbe': 'hardware_info_probe',
                        'AppUsageProbe': 'app_usage_probe'}

If it's just because the first projects to use this have funf-based, that's fine, but it seems to me that it's in the project's best interest to decouple it entirely and abstract the datastore objects from the kind of data being stored.

If it's part of some design decision I'm not privy to, could you explain it to me?

thanks!

RogerTangos commented 9 years ago

Hey Dan - it really should be decoupled. The only reason things are so non-orthogonal is that it was originally made with funf in mind.

Albert Carter

Programmer Big Data Initiative CSAIL, MIT

On Thu, Jan 22, 2015 at 4:37 PM, Dan Calacci notifications@github.com wrote:

Hey, new to the project, been cleaning up the code and migrating the codebase to django 1.7 and other stuff like that on my own fork.

I'm wondering why some modules - AccessControlledInternalDataStore in particular - are coupled so tightly with Funf. For example, that class has a mapping field that directly relates to funf data:

    self.mapping = {'ActivityProbe': 'activity_probe',
                    'SmsProbe': 'sms_probe',
                    'CallLogProbe': 'call_log_probe',
                    'BluetoothProbe': 'bluetooth_probe',
                    'WifiProbe': 'wifi_probe',
                    'LocationProbe': 'simple_location_probe',
                    'ScreenProbe': 'screen_probe',
                    'RunningApplicationsProbe': 'running_applications_probe',
                    'HardwareInfoProbe': 'hardware_info_probe',
                    'AppUsageProbe': 'app_usage_probe'}

If it's just because the first projects to use this have funf-based, that's fine, but it seems to me that it's in the project's best interest to decouple it entirely and abstract the datastore objects from the kind of data being stored.

If it's part of some design decision I'm not privy to, could you explain it to me?

thanks!

— Reply to this email directly or view it on GitHub https://github.com/HumanDynamics/openPDS/issues/38.

brian717 commented 9 years ago

To add to Al's comment - the whole AccessControl package is a bit messy, and a rewrite would do it some good. At the moment, most deployments ignore it completely (control via scopes is typically good enough).

It currently uses a model / table with column names corresponding to different probe names in Funf, and this is where the mapping dictionary comes into play. It's not quite coupled to Funf (the data could come from anywhere), but it does use the naming convention from Funf (not ideal, but not too bad), and is fixed in the number of different data types it can support (a worse problem than the column names).

A better way to do this (and remove the Funf naming convention) would be to store these settings as a dictionary in either the PDS backend (configurable independent of the Django ORM) or the Django ORM (as a property bag), with data types / probes as keys.

Either storage method would work, if you're interested in taking this on. The ORM property bag is likely the faster route.

dcalacci commented 9 years ago

hey, not 100% sure what you were thinking of when you said 'property bag', but I took a stab at it.

wanted to see if what I did is agreeable before I go and update other parts of the code.

brian717 commented 9 years ago

This is almost exactly what I meant, though by property bag I was thinking of a separate table for keys / datatype names, and a table linking a settings object to a key / attrib to a value. What you've done achieves the same thing, and if attrib names, app_id, and lab_id are unique and we don't plan on changing them after the fact, it works just as well anyway.

So... thumbs-up from me. Definitely a positive change.

dcalacci commented 9 years ago

what you just described is nice and extensible, but this seems like it will work for now, I'll move forward

:joy: