search

cleverhans-lab / cleverhans

An adversarial example library for constructing attacks, building defenses, and benchmarking both
MIT License
6.2k stars 1.39k forks source link

Accept sess=None in attacks #856

Closed erfannoury closed 6 years ago

erfannoury commented 6 years ago

With the current implementation, every object created from the classes based on the Attack class need to provide a tf.Session instance in the constructor. However, this limits the usage of the attacks to cases where a Session has already been created. For instance, there are problems for use cases in which the attacks are used in the graph construction phase and tf.Session will be created later and will not be accessible to the user. This happens when using the Estimator API in which attacks can either be used for training or for evaluation. In both cases the graph will be constructed first and creating the session and executing the graph is handled by the Estimator class. Would it be reasonable to move this check to where sess actually will be used?

goodfeli commented 6 years ago

Is sess even used anywhere other than generate_np? It might make more sense to just take sess out of the Attack class altogether, and have generate_np take a sess argument that defaults to tf.get_default_session() or something like that.

AlexeyKurakin commented 6 years ago

I looked through all the code of the attacks. Most of the attack only use sess in the generate_np. However few attacks don't have proper symbolic implementation (CW, EAD, DeepFool, LBFGS) and rely on sess in generate method. Plus one attack (SaliencyMaps) has a flag which controls whether generate uses pure symbolic attack or uses pyfunc with session.

The main problem with current implementation is that __init__ method of attack class always assigns self.sess even if session was not provided to constructor.

So I would say ideal solution would be to rewrite all remaining attacks to have proper symbolic implementation and then move session argument from the constructor to generate_np. However modifying these few attacks is quite a lot of efforts.

Less ideal solution could be following. We can allow init to accept None as the sess and remove the code which creates default session from the attack classes. And we put assert self.sess is not None into generate method of those few attacks which does need session.

erfannoury commented 6 years ago

Second solution would be much easier, but I don't know whether it's the preferred method. Is there a way I can help make any of these changes?

AlexeyKurakin commented 6 years ago

I think it's fine to do second solution for now.

AlexeyKurakin commented 6 years ago

@erfannoury if you willing to make this change it would be great

erfannoury commented 6 years ago

@AlexeyKurakin Sure, I can start working on them and will submit a PR as soon as possible.

AlexeyKurakin commented 6 years ago

@erfannoury Thanks for implementing this change. Your PR should resolve this issue (except for small subset of attacks), so closing it for now. Feel free to reopen if you have any additional concerns