pepkit / peppy

Project metadata manager for PEPs in Python
https://pep.databio.org/peppy
BSD 2-Clause "Simplified" License
37 stars 12 forks source link

Remove attmap dependency #447

Closed nsheff closed 7 months ago

nsheff commented 1 year ago

As in other projects, we should remove attmap here.

khoroshevskyi commented 1 year ago

After removal of attmap, we won't have one of the features of retrieving and storing and assigning the variables. Now with attmap we can assign variables to Sample class using 2 ways: 1)

sample.some_key = "some value"

2)

sample["some_key"] = "some value"

The same with getting values from object:

# 1)
sample.some_key 
# 2)
sample["some_key"] 

I think, it's possible to use two methods to get values, but only way of assigning and storing them. We can store variables as key values pairs or as attributes. What option is better for us in this case?

nsheff commented 1 year ago

we should remove the ability to either assign or store values using the dot notation, and require bracket notation for both.

nsheff commented 1 year ago

I"m still trying to track this down, but the .config attribute on a Project is sometimes a YAMLConfigManager object, which means the Project.to_dict() needs to call the .config.to_dict() methodl...

but this causes some errors because sometimes the .config is actually just a dict, not a YAMLCOnfigManager. So, still need to get to the bottom of that.

khoroshevskyi commented 1 year ago

I"m still trying to track this down, but the .config attribute on a Project is sometimes a YAMLConfigManager object, which means the Project.to_dict() needs to call the .config.to_dict() methodl...

but this causes some errors because sometimes the .config is actually just a dict, not a YAMLCOnfigManager. So, still need to get to the bottom of that.

I changed every object that stores config to dict type. This resolved the failing tests and errors in eido. We no longer use YAMLConfigManager. The issue should be resolved in this commit:https://github.com/pepkit/peppy/pull/448/commits/473d3a7388d4a0bb6a113a4b961b4e58ac6ca2f5

khoroshevskyi commented 11 months ago

Yesterday I released pre-release of peppy: https://pypi.org/project/peppy/0.40.0a1/ Me and @donaldcampbelljr tried to use it with looper and unfortunately it was broken (everything was broken). Why: Because looper class Project inherits peppy.Project and sets some additional attributes (e.g. setattr(self, "NewAttr")) and then tries to call it using items (e.g. self["NewAttr"] ) This functionality is broken, because you cannot mix attributes and items in Project class. This is how I implemented it

Sample class has this functionality and you can mix attributes and items. If I remember correctly, I didn't add this functionality to Project class because of the nesting of the objects.. and I wanted to keep it simple.

Solution: 1) Keep peppy as it is and fix looper 2) Try to fix Project using Mapping 3) Fix Project by inheriting dict

@nsheff

nsheff commented 11 months ago

I vote for 1. If looper uses setattr, then it should use attr access to access those attributes. Or, it should change to using setitem and then use item access.

it should not mix the two.

khoroshevskyi commented 11 months ago

update (Aug 21 2023): issue is solved and all changes are on dev branch