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

Look at the full collection of Exchange classes #1761

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 12 additions & 22 deletions nbgrader/apps/baseapp.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,35 +149,25 @@ def all_configurable_classes(self) -> TypingList[MetaHasTraits]:
if len(app.class_traits(config=True)) > 0:
classes.append(app)

def _collect_configurables(module):
"""Return a list of all configurable classes from a module"""

for name in module.__all__:
cls = getattr(module, name)
if hasattr(cls, "class_traits") and cls.class_traits(config=True):
classes.append(cls)

# include plugins that have configurable options
for pg_name in plugins.__all__:
pg = getattr(plugins, pg_name)
if pg.class_traits(config=True):
classes.append(pg)
_collect_configurables(plugins)

# include all preprocessors that have configurable options
for pp_name in preprocessors.__all__:
pp = getattr(preprocessors, pp_name)
if len(pp.class_traits(config=True)) > 0:
classes.append(pp)

# include all the exchange actions
for ex_name in exchange.__all__:
ex = getattr(exchange, ex_name)
if hasattr(ex, "class_traits") and ex.class_traits(config=True):
classes.append(ex)
_collect_configurables(preprocessors)

# include all the default exchange actions
for ex_name in exchange.default.__all__:
ex = getattr(exchange, ex_name)
if hasattr(ex, "class_traits") and ex.class_traits(config=True):
classes.append(ex)
_collect_configurables(exchange.default)
Copy link
Contributor

@brichet brichet Jun 12, 2023

Choose a reason for hiding this comment

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

Suggested change
_collect_configurables(exchange.default)
_collect_configurables(exchange.default.exchange)

Do you think we need to catch all the default classes traits if they are similar to the abc ones ?
Since exchange is the only default class with dedicated traits, maybe it can be specified when calling the _collect_configurables function.
This should avoid duplication for most of the exchange classes, but I'm not sure it will not break anything about the functionalities, I don't know well traitlets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After digging into this more, I think the duplication is just the nature of the beast. I have a new proposal to make the distinction more clear, which is to actually rename the abc classes.

When they're used in the nbgrader codebase, we already do from exchange.abc import Exchange as ABCExchange, and if you search GitHub, so does everyone else: https://github.com/search?q=from+nbgrader.exchange+import+&type=code

By renaming the actual class, we can get rid of these import aliases without touching the implementation, and create distinction in the generated config file.


# include all the converters
for ex_name in converters.__all__:
ex = getattr(converters, ex_name)
if hasattr(ex, "class_traits") and ex.class_traits(config=True):
classes.append(ex)
_collect_configurables(converters)

return classes

Expand Down
3 changes: 2 additions & 1 deletion nbgrader/apps/collectapp.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
from traitlets import default

from .baseapp import NbGrader, nbgrader_aliases, nbgrader_flags
from ..exchange import Exchange, ExchangeCollect, ExchangeError
from ..exchange.default import Exchange, ExchangeCollect
from ..exchange import ExchangeError


aliases = {}
Expand Down
39 changes: 23 additions & 16 deletions nbgrader/exchange/__init__.py
Original file line number Diff line number Diff line change
@@ -1,21 +1,28 @@

from nbgrader.exchange.abc import (Exchange, ExchangeError, ExchangeCollect, ExchangeFetch, ExchangeFetchAssignment,
ExchangeFetchFeedback, ExchangeList, ExchangeReleaseAssignment, ExchangeRelease,
ExchangeReleaseFeedback, ExchangeSubmit, ExchangeReleaseFeedback)
import warnings
from nbgrader.exchange.abc import ExchangeError
from nbgrader.exchange import abc, default
from .exchange_factory import ExchangeFactory

def __getattr__(name):
if name in abc.__all__:
warnings.warn(
f"Importing {name} from nbgrader.exchange is deprecated."
" Import from nbgrader.exchange.abc or the specific "
" exchange implementation instead.".format(name),
DeprecationWarning,
stacklevel=2
)

if hasattr(abc, name):
return getattr(abc, name)
elif hasattr(default, name):
return getattr(default, name)

raise AttributeError(f"module {__name__!r} has no attribute {name!r}")

__all__ = [
"Exchange",
"ExchangeError",
"ExchangeCollect",
"ExchangeFetch",
"ExchangeFetchAssignment",
"ExchangeFetchFeedback",
"ExchangeList",
"ExchangeRelease",
"ExchangeReleaseAssignment",
"ExchangeReleaseFeedback",
"ExchangeSubmit",
"abc",
"default",
"ExchangeFactory",
"default"
"ExchangeError",
]
20 changes: 10 additions & 10 deletions nbgrader/exchange/abc/__init__.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
from .exchange import ExchangeError, Exchange
from .submit import ExchangeSubmit
from .release_feedback import ExchangeReleaseFeedback
from .release import ExchangeRelease
from .release_assignment import ExchangeReleaseAssignment
from .fetch_feedback import ExchangeFetchFeedback
from .fetch import ExchangeFetch
from .fetch_assignment import ExchangeFetchAssignment
from .collect import ExchangeCollect
from .list import ExchangeList
from .exchange import ExchangeError, ABCExchange as Exchange
from .submit import ABCExchangeSubmit as ExchangeSubmit
from .release_feedback import ABCExchangeReleaseFeedback as ExchangeReleaseFeedback
from .release import ABCExchangeRelease as ExchangeRelease
from .release_assignment import ABCExchangeReleaseAssignment as ExchangeReleaseAssignment
from .fetch_feedback import ABCExchangeFetchFeedback as ExchangeFetchFeedback
from .fetch import ABCExchangeFetch as ExchangeFetch
from .fetch_assignment import ABCExchangeFetchAssignment as ExchangeFetchAssignment
from .collect import ABCExchangeCollect as ExchangeCollect
from .list import ABCExchangeList as ExchangeList

__all__ = [
"Exchange",
Expand Down
4 changes: 2 additions & 2 deletions nbgrader/exchange/abc/collect.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
from traitlets import Bool

from .exchange import Exchange
from .exchange import ABCExchange


class ExchangeCollect(Exchange):
class ABCExchangeCollect(ABCExchange):

update = Bool(
False,
Expand Down
15 changes: 10 additions & 5 deletions nbgrader/exchange/abc/exchange.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import datetime
from abc import abstractmethod

from textwrap import dedent

Expand All @@ -15,7 +16,11 @@ class ExchangeError(Exception):
pass


class Exchange(LoggingConfigurable):
class ABCExchange(LoggingConfigurable):
"""
The abstract Exchange, which underlies every step in the exchange process.
"""

assignment_dir = Unicode(
".",
help=dedent(
Expand Down Expand Up @@ -51,7 +56,7 @@ def _valid_timestamp_format(self, proposal):
def __init__(self, coursedir=None, authenticator=None, **kwargs):
self.coursedir = coursedir
self.authenticator = authenticator
super(Exchange, self).__init__(**kwargs)
super(ABCExchange, self).__init__(**kwargs)

def fail(self, msg):
self.log.fatal(msg)
Expand All @@ -64,17 +69,17 @@ def set_timestamp(self):
self.fail("Invalid timezone: {}".format(self.timezone))
self.timestamp = datetime.datetime.now(tz).strftime(self.timestamp_format)

@abstractmethod
def init_src(self):
"""Compute and check the source paths for the transfer."""
raise NotImplementedError

@abstractmethod
def init_dest(self):
"""Compute and check the destination paths for the transfer."""
raise NotImplementedError

@abstractmethod
def copy_files(self):
"""Actually do the file transfer."""
raise NotImplementedError

def start(self):
self.set_timestamp()
Expand Down
10 changes: 5 additions & 5 deletions nbgrader/exchange/abc/fetch.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import warnings

from .fetch_assignment import ExchangeFetchAssignment
from .fetch_assignment import ABCExchangeFetchAssignment


class ExchangeFetch(ExchangeFetchAssignment):
class ABCExchangeFetch(ABCExchangeFetchAssignment):

def __init__(self, *args, **kwargs):
super(ExchangeFetch, self).__init__(*args, **kwargs)
super(ABCExchangeFetch, self).__init__(*args, **kwargs)
msg = (
"The ExchangeFetch class is now deprecated, please use "
"ExchangeFetchAssignment instead. This class will be removed in a "
"The ABCExchangeFetch class is now deprecated, please use "
"ABCExchangeFetchAssignment instead. This class will be removed in a "
"future version of nbgrader.")
warnings.warn(msg, DeprecationWarning)
self.log.warning(msg)
4 changes: 2 additions & 2 deletions nbgrader/exchange/abc/fetch_assignment.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
from traitlets import Bool

from .exchange import Exchange
from .exchange import ABCExchange


class ExchangeFetchAssignment(Exchange):
class ABCExchangeFetchAssignment(ABCExchange):

replace_missing_files = Bool(False, help="Whether to replace missing files on fetch").tag(config=True)
4 changes: 2 additions & 2 deletions nbgrader/exchange/abc/fetch_feedback.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from .exchange import Exchange
from .exchange import ABCExchange


class ExchangeFetchFeedback(Exchange):
class ABCExchangeFetchFeedback(ABCExchange):
pass
6 changes: 3 additions & 3 deletions nbgrader/exchange/abc/list.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
from traitlets import Bool
from .exchange import Exchange
from .exchange import ABCExchange


class ExchangeList(Exchange):
class ABCExchangeList(ABCExchange):

inbound = Bool(False, help="List inbound files rather than outbound.").tag(config=True)
cached = Bool(False, help="List assignments in submission cache.").tag(config=True)
Expand All @@ -20,7 +20,7 @@ def start(self):
if self.inbound and self.cached:
self.fail("Options --inbound and --cached are incompatible.")

super(ExchangeList, self).start()
super(ABCExchangeList, self).start()

if self.remove:
return self.remove_files()
Expand Down
10 changes: 5 additions & 5 deletions nbgrader/exchange/abc/release.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import warnings

from .release_assignment import ExchangeReleaseAssignment
from .release_assignment import ABCExchangeReleaseAssignment


class ExchangeRelease(ExchangeReleaseAssignment):
class ABCExchangeRelease(ABCExchangeReleaseAssignment):

def __init__(self, *args, **kwargs):
super(ExchangeRelease, self).__init__(*args, **kwargs)
super(ABCExchangeRelease, self).__init__(*args, **kwargs)
msg = (
"The ExchangeRelease class is now deprecated, please use "
"ExchangeReleaseAssignment instead. This class will be removed in "
"The ABCExchangeRelease class is now deprecated, please use "
"ABCExchangeReleaseAssignment instead. This class will be removed in "
"a future version of nbgrader.")
warnings.warn(msg, DeprecationWarning)
self.log.warning(msg)
4 changes: 2 additions & 2 deletions nbgrader/exchange/abc/release_assignment.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
from traitlets import Bool

from .exchange import Exchange
from .exchange import ABCExchange


class ExchangeReleaseAssignment(Exchange):
class ABCExchangeReleaseAssignment(ABCExchange):

force = Bool(False, help="Force overwrite existing files in the exchange.").tag(config=True)
4 changes: 2 additions & 2 deletions nbgrader/exchange/abc/release_feedback.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from .exchange import Exchange
from .exchange import ABCExchange


class ExchangeReleaseFeedback(Exchange):
class ABCExchangeReleaseFeedback(ABCExchange):
pass
4 changes: 2 additions & 2 deletions nbgrader/exchange/abc/submit.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
from textwrap import dedent
from traitlets import Bool

from .exchange import Exchange
from .exchange import ABCExchange


class ExchangeSubmit(Exchange):
class ABCExchangeSubmit(ABCExchange):

strict = Bool(
False,
Expand Down
4 changes: 4 additions & 0 deletions nbgrader/exchange/default/exchange.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@


class Exchange(ABCExchange):
"""
The default implementation of an Exchange based on the local file system.
"""

root = Unicode(
"/usr/local/share/nbgrader/exchange",
help="The nbgrader exchange directory writable to everyone. MUST be preexisting."
Expand Down