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

WIP: Spring cleaning (Treant unification, no uuids, .datreant state) #145

Merged
merged 43 commits into from Nov 14, 2017

Conversation

Projects
None yet
6 participants
@dotsdl
Copy link
Member

commented Apr 24, 2017

fixes #100
fixes #101
fixes #135
fixes #65

A couple weeks ago @kain88-de and @richardjgowers and I met to discuss some changes I was considering for datreant. These changes include removal of uuids (#135), removal of the concept of different Treant types from the whole library (goes toward #100), and the replacement of a bare state file with a .datreant directory indicating a Treant.

I'd like to store tags and categories in separate files under this scheme, which will require more hacking. Basically, instead of a limb such as Tags storing all its stuff with any other limb's stuff in the same file, it can do what it needs separately. Not sure if this idea will be something we like but toying with it still.

Once the dust settles I'll also add a converter script for putting an existing Treant into the new form.

@dotsdl dotsdl added this to the 0.8.0 milestone Apr 24, 2017

@dotsdl dotsdl requested review from kain88-de and richardjgowers Apr 24, 2017

dotsdl added some commits May 5, 2017

Now tags and categories have their own state file.
There is no state file for the overall Treant anymore. Now, what makes a
Treant is the presence of the `.datreant` directory alone. Limbs make
whatever files they need inside there to function.
Using __getattribute__ requires __setstate__ gymnastics.
All tests pass again, so we shouldn't have any obvious regressions.
Now need to add tests that Treants are as hardy as they should be with
new machinery.

@property
def _treantfile(self):
return os.path.join(self.abspath, treantdir_name, treantfile_name)

This comment has been minimized.

Copy link
@kain88-de

kain88-de May 8, 2017

Contributor

return os.path.join(self._treantdir, treantfile_name)

"""
return self._backend.filename
return Path(self._backend.get_location()).absolute().parent

This comment has been minimized.

Copy link
@kain88-de

kain88-de May 8, 2017

Contributor

why not Path(self._treantdir).absolute().parent?

@kain88-de
Copy link
Contributor

left a comment

Looks good so far

@@ -0,0 +1,2 @@
treantdir_name = '.datreant'
treantfile_name = 'state.json'

This comment has been minimized.

Copy link
@kain88-de

kain88-de May 9, 2017

Contributor

shouldn't globals we written in all caps

Added treeloc, leafloc path accessors to Tree, View, Bundle.
These aren't tested yet, so much to do still.
@dotsdl

This comment has been minimized.

Copy link
Member Author

commented May 28, 2017

Status update: this will be a big PR, essentially being a refresh of much of the core library. I went through the list of open issues last night, and the changes we're making here will make many of our older issues either solved or irrelevant. This is similar in effect (though not in scale) to the MDAnalysis topology system refactor, and I'm shooting for consistency throughout the library.

Hang tight; more to come. :D

@codecov-io

This comment has been minimized.

Copy link

commented May 28, 2017

Codecov Report

Merging #145 into develop will increase coverage by 1.93%.
The diff coverage is 80.29%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #145      +/-   ##
===========================================
+ Coverage     77.7%   79.64%   +1.93%     
===========================================
  Files           14       12       -2     
  Lines         1624     1243     -381     
  Branches       354      283      -71     
===========================================
- Hits          1262      990     -272     
+ Misses         306      206     -100     
+ Partials        56       47       -9
Impacted Files Coverage Δ
src/datreant/core/__init__.py 100% <ø> (ø) ⬆️
src/datreant/core/util.py 100% <100%> (ø) ⬆️
src/datreant/core/exceptions.py 100% <100%> (ø)
src/datreant/core/names.py 100% <100%> (ø)
src/datreant/core/manipulators.py 87.5% <100%> (-1.08%) ⬇️
src/datreant/core/state.py 71.81% <100%> (ø)
src/datreant/core/collections.py 67.68% <61.41%> (-0.29%) ⬇️
src/datreant/core/trees.py 84.31% <66.66%> (+0.38%) ⬆️
src/datreant/core/treants.py 74.11% <71.42%> (-9.56%) ⬇️
src/datreant/core/metadata.py 90.36% <94.08%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dca253e...5a2d993. Read the comment docs.

@kain88-de

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2017

@dotsdl any progress on this?

@dotsdl

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2017

@kain88-de been working on-and-off on it. Have a new application to build for work that I think will be MDSynthesis-like, in that it uses Treant as a base-class for a domain-specific object. We use datreant pretty extensively for getting things done there, but I don't have a lot of spare cycles at the moment to work directly on it.

I think when @richardjgowers is here for a week I'll find some motivation to just finish it. Standby. :D

dotsdl and others added some commits Jul 16, 2017

remove os library usage
use pathlib directly

@kain88-de kain88-de force-pushed the spring_cleaning branch from 8e1cc05 to 2adcc2d Jul 21, 2017

kain88-de added some commits Jul 21, 2017

use python 3.6 API of os.makedirs
This will allow to drop the wrapper once the library is py3 only.
@kain88-de

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

funny the code now works on python 3.6 and only that version

@kain88-de

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

The error seems to be with the multiprocessing module. I don't have any experience with it myself. @dotsdl would be nice if you could have a lock

@kain88-de

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

OK I fixed the python 2.7 errors I found as well. But really no clue about the multiprocessing errors. It looks like the processes never finish

setup.py Outdated
@@ -32,5 +32,5 @@
license='BSD',
long_description=open('README.rst').read(),
tests_require = ['numpy', 'pytest>=2.10', 'mock'],
install_requires=['asciitree', 'pathlib2', 'scandir', 'six', 'fuzzywuzzy']
install_requires=['asciitree', 'pathlib2', 'scandir', 'six', 'fuzzywuzzy', 'six']

This comment has been minimized.

Copy link
@jbarnoud

jbarnoud Jul 21, 2017

'six' appears twice.

This comment has been minimized.

Copy link
@kain88-de

kain88-de Jul 21, 2017

Contributor

thanks I overlooked that

Limbs are generally applicable to Treants and Trees.
Small changes to make this clearer, even if not all limbs are suitable
for Tree objects.
@dotsdl

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2017

@orbeckst don't worry...got Python 2.7 working again. Was a small issue with how pickling was being handled in 2 vs. 3. We won't be dropping support at this time.

@kain88-de

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2017

@dotsdl and @richardjgowers do you have a list of what is still left todo?

@kain88-de kain88-de force-pushed the spring_cleaning branch from d919b34 to 924cba3 Aug 21, 2017

@kain88-de kain88-de force-pushed the spring_cleaning branch from 924cba3 to a19006f Aug 21, 2017

kain88-de and others added some commits Aug 22, 2017

from .trees import Tree, Leaf
from .names import treantdir_name

This comment has been minimized.

Copy link
@kain88-de

kain88-de Oct 9, 2017

Contributor

I'm still for using all caps on constants

"""
_classagglimbs = set()
_agglimbs = set()

def __init__(self, *vegs, **kwargs):

This comment has been minimized.

Copy link
@kain88-de

kain88-de Oct 10, 2017

Contributor

kwargs are unused now. Still needed ?

def __init__(self, collection):
self._collection = collection
@staticmethod
def _init_state(jsonfile):

This comment has been minimized.

Copy link
@kain88-de

kain88-de Oct 10, 2017

Contributor

why is this function really needed? I don't see the benefit of it.

This comment has been minimized.

Copy link
@kain88-de

kain88-de Oct 10, 2017

Contributor

I'm also confused here why this function is static. It is only ever called with self as argument.

This comment has been minimized.

Copy link
@dotsdl

dotsdl Nov 14, 2017

Author Member

This method is the init_state method specific to Tags, which gets fed to the JSONFile constructor for use when creating a file from scratch. For Tags this generates an empty list. Categories has its own that generates an empty dict.

super(JSONFile, self).__init__(filename, **kwargs)

# set _init_state function that given
self._init_state = init_state

This comment has been minimized.

Copy link
@kain88-de

kain88-de Oct 10, 2017

Contributor

the object self._init_state is nowhere really called.

This comment has been minimized.

Copy link
@dotsdl

dotsdl Nov 14, 2017

Author Member

self._init_state gets called by Tags and Categories classes, which use JSONFile to talk to their state files. Basically, it's defined at runtime on creation of the JSONFile object.

I'll admit it's a confusing way to go about it. I'm open to alternatives.

This comment has been minimized.

Copy link
@kain88-de

kain88-de Nov 14, 2017

Contributor

yeah, but where is it called? It should also be commented what the intent of this. Otherwise, I'm gonna ask again in the future.

This comment has been minimized.

Copy link
@dotsdl

dotsdl Nov 14, 2017

Author Member

I added a brief docstring to each with this information. Is there more needed, though?

This comment has been minimized.

Copy link
@kain88-de

kain88-de Nov 14, 2017

Contributor

Nope clearer now. A comment here may be nice. But I don't remember exactly why I didn't understand this. Reading the code now it's clear to me.

"""Return a set giving the Tags in `a` that are not in `b`.
"""
from .metadata import AggTags

This comment has been minimized.

Copy link
@kain88-de

kain88-de Oct 10, 2017

Contributor

This line is cool. You are saying import class from this file.

This comment has been minimized.

Copy link
@dotsdl

dotsdl Nov 14, 2017

Author Member

Ha...that's an artifact from before when these classes where in a separate file. I can probably get rid of it.

dotsdl added some commits Nov 14, 2017

Added exceptions for when `.datreant` directory is missing.
Added thee upon access of tags or categories since these throw
exceptions already upon access. We don't want to do a check for
`.datreant` on every access of Treant since this will have a potentially
high performance hit for many Treants. Basically, we only raise the
issue when it really matters.
@dotsdl

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2017

Finished updating docs. Next up is to write a simple converter script for old Treants to new form.

@kain88-de

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2017

Finished updating docs. Next up is to write a simple converter script for old Treants to new form.

@mimischi there is gonna be a script, I hope I can adjust it to work on a Sim as well.

This means that changing the location or name of a Treant can be done at the
filesystem level. Although this means that one can change the treanttype and
uuid as well, this is generally not recommended.
place on disk; they store almost nothing in memory.

This comment has been minimized.

Copy link
@kain88-de

kain88-de Nov 14, 2017

Contributor

Please also point out that we only keep the filehandle alive as shortly as possible. It's good on rare occasions to have this documented.

This comment has been minimized.

Copy link
@dotsdl

dotsdl Nov 14, 2017

Author Member

Good call. Added.

@dotsdl

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2017

If we're happy with this, I'm good with merging (finally). Next up will be getting the other PRs that have been waiting in the wings finally in as well.

@kain88-de

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2017

sure merge ahead

@dotsdl dotsdl merged commit 830bef2 into develop Nov 14, 2017

3 checks passed

codecov/patch 80.29% of diff hit (target 77.7%)
Details
codecov/project 79.64% (+1.93%) compared to dca253e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kain88-de kain88-de deleted the spring_cleaning branch Nov 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.