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

Initial Implementation of a Data class #177

Merged
merged 9 commits into from
Jan 29, 2020

Conversation

tjduigna
Copy link
Contributor

@tjduigna tjduigna commented Jan 26, 2020

Closes #167

Copy link
Member

@avmarchenko avmarchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm my understanding: if we used HasTraits as a base class we would only get the effect of typing and default values for attributes ("traits") - we wouldn't be able to leverage the configuration features correct?

@tjduigna
Copy link
Contributor Author

tjduigna commented Jan 26, 2020

Yes, as far as I understand it, HasTraits is a base class for Configurable, which additionally supplies the configuration machinery. It is probably the case that some Base classes need only HasTraits functionality because rather than relying on configurable trait values we can always have a dynamic default.

@tjduigna tjduigna added the 1.0 label Jan 27, 2020
@tjduigna tjduigna changed the title WIP: Implementation of a Data class Initial Implementation of a Data class Jan 27, 2020
@tjduigna
Copy link
Contributor Author

I updated #167 to reflect the development here.

exa/core/data.py Outdated Show resolved Hide resolved
exa/core/data.py Outdated
else: # assume this is a function
source = getattr(mod, obj)
except Exception as e:
print("attempt to import source failed")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we log or print or warn the traceback/exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah just printing is lazy. Should we go with self.log.error(e) to get a one-liner of the exception or self.log.exception which will give us the stack trace as well?

raise TraitError("source must be callable")
return prop['value']

@observe('source')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this use of these event triggers.

*mod, obj = source.split('.')
mod = importlib.import_module('.'.join(mod))
# TODO : do we also support providing init
# args and kwargs in case source is class?
Copy link
Member

@avmarchenko avmarchenko Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the tests I see source_args and kwargs. Were those intended to be passed to the constructor of the class? Is functools.partial something we could use here to ship args for callables (taking a bit of functional programming possibly)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes this is something I mentioned in #167 perhaps we should move the design discussion to the Issue. I will post a response there.

@@ -0,0 +1,14 @@
FROM tensorflow/tensoflow:2.0.0-py3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is part of #176 right?

@tjduigna tjduigna merged commit dee2d97 into exa-analytics:feature/1.0 Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants