unity-sds / unity-py

Apache License 2.0
2 stars 4 forks source link

Feature/client work #4

Closed mike-gangl closed 1 year ago

mike-gangl commented 1 year ago

this is a big review, but i wanted as many eyes on it before i went down a bad path.

This essentially creates the unity separation of services:

and their resources:

Not all of it is done, but there are the bones of what we're trying to do that is similar to boto3 construction.

Currently a session is created which handles configuration and auth, that session can be used to create a service:

s = Session()

that will look for or promtp for auth as needed, which can then be shared with all of the other services.

then, you'd use the session (and it may be renamed to "unity" to get the service abstraction:

dataManager = s.client("DataManager")
collections = dataManager.get_collections()
print(collections)

that's all tehre is to making a cognito authenticated call to the DAPA endpoint. Those collections can then be used to query data:

cd = dataManager.get_collection_data(collections[0])
for dataset in cd:
    print(f'dataset name: {dataset.id}' )
    for f in dataset.datafiles:
        print("\t" + f.location)

Again proof of concept, but it's a lot of work to get these items in place, and wanted to discuss this before going down another, longer path.

Some topics:

anilnatha commented 1 year ago

Read through your description, still going through the code....

anilnatha commented 1 year ago

Hey @mike-gangl ,

I've been studying what you've created and have some thoughts.

I like what you've done with using session object that the user interacts with for authn/authz. This object could also be used to fetch information about their profile, what they have access to, and other user profile related functionalities we want to add in the future.

  1. Could we simplify this by having users instantiate their 'manager' objects rather than using the client method to fetch a manager object from the Unity or Session class? We can then get rid of the client method in Session.

    from unity_py import Session, DataManager
    
    s = Session()
    data_manager = DataManager(s)

OR

  1. Rather than calling it client, rename it to login, rename managers to service and we get:
s = Session()
data_service = s.login('DataService')
data_service.get_collections()
...
  1. Also, seeing your code for session, and the Unity class I originally developed, I think we should combine them.
mike-gangl commented 1 year ago

i do not like the second option- that's just a gut reaction- i'd prefer the session object being passed be transparent to the end user (even though it's done explicitly in the code, the user doesn't "care" about that).

In terms of the first option, i think that's a fair way of doing it, but can i suggest a 3rd?

Rename "session" to Unity, and we can make it more explicit:

unity = Unity()
data_service = unity.get_data_manager()
data_service.get_collections()

# also available
# application_service = unity.get_application_manager()
# process_service = unity.get_process_manager()

To me this makes the "passing" of a session object transparent to the user, which i think is ideal, but there could be a world in which we want to interact without a session, but at that point, mayb we can add more args to the Unity() call to explicitly not login or not look for auth.

anilnatha commented 1 year ago

@mike-gangl Committed updates to the repo for your review. tests are failing so I need to look into those. When I run the tests locally, no errors are reported.

(venv) anatha@MT-110191 unity_py % poetry run pytest -s                 
================================================================================================================================================= test session starts ==================================================================================================================================================
platform darwin -- Python 3.9.6, pytest-7.1.3, pluggy-1.0.0
rootdir: /Users/anatha/Projects/Unity/Repos/unity-py, configfile: pyproject.toml
collected 0 items                                                                                                                                                                                                                                                                                                      

================================================================================================================================================ no tests ran in 0.01s =================================================================================================================================================
(venv) anatha@MT-110191 unity_py % poetry run pytest -m "not regression"
================================================================================================================================================= test session starts ==================================================================================================================================================
platform darwin -- Python 3.9.6, pytest-7.1.3, pluggy-1.0.0
rootdir: /Users/anatha/Projects/Unity/Repos/unity-py, configfile: pyproject.toml
collected 0 items                                                                                                                                                                                                                                                                                                      

================================================================================================================================================ no tests ran in 0.01s =================================================================================================================================================
(venv) anatha@MT-110191 unity_py % 
anilnatha commented 1 year ago

Notable changes:

  1. Removed old Unity class and renamed your Session class to Unity.
  2. Addition of UnityServices class to provide consistent selection of a service.
  3. Addition of UnityEnvironments class to provide consistent selection of an environment.
  4. Renamed DataManager to DataService and moved to services folder.

I also tried to get pdoc working but it needs some tweaking to display the docs correctly, so will work on that more later.

mike-gangl commented 1 year ago

Are you using environment variables for the testing locally? the failures are happening because it's asking the suer for input.

mike-gangl commented 1 year ago

because we replaced UnitySession() with Unity(), this test line is now failing:

https://github.com/unity-sds/unity-py/blob/f185b6b736d99b7113d47f9c152036fa0d847b27/tests/test_unity_py.py#L13