-
Notifications
You must be signed in to change notification settings - Fork 138
dffml: cli: Pass model specific config options #31
Conversation
I have tried to fetch -modelParams value but i'm getting the following error:
I have made changes in |
Codecov Report
@@ Coverage Diff @@
## master #31 +/- ##
==========================================
- Coverage 88.97% 88.64% -0.34%
==========================================
Files 46 47 +1
Lines 3230 3250 +20
Branches 336 340 +4
==========================================
+ Hits 2874 2881 +7
- Misses 306 318 +12
- Partials 50 51 +1
Continue to review full report at Codecov.
|
Fixed the previous issue. It was due to an indentation error. I'm currently working on fetching the params inside dnn model. |
Hey Yash!
I haven't modified dffml/cli.py. I think this requires changes in
dffml/util/cli.py mainly to fetch the value.
Please let me know if you have any suggestions.
Regards,
…On Fri, Mar 22, 2019 at 5:57 PM Yash Lamba ***@***.***> wrote:
Hey! @sudharsana-kjl <https://github.com/sudharsana-kjl>, did you modify
dffml/cli.py? I maybe wrong but I don't see any modifications to that.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#31 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AJpIdVxmIbzwMLp4TOLYc4UdrHKJipztks5vZMwogaJpZM4cDZSk>
.
|
Yeah, I saw that later. Is the error resolved now? |
Hey Yash!
Yes. The error has been resolved now. I'm working on implementing the
feature.
Thanks!
…On Fri, 22 Mar, 2019, 7:51 PM Yash Lamba, ***@***.***> wrote:
Yeah, I saw that later. Is the error resolved now?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#31 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AJpIdfoZBfCBK8amDMAwy4cnpeg3eJr9ks5vZObagaJpZM4cDZSk>
.
|
You're on the right track! Apologies for the lack of comments in that file ( and well, all of them ) I think you may need to add a sub parser or a group or something.
Lines 168 to 169 in f2a42ca
Also, you'll want to change the model action so that the second () is used to pass the arguments parsed in via the sub parser or whatever Lines 37 to 40 in f2a42ca
So: Model.load(value)(**dict_of_args_for_model) |
Hey @pdxjohnny! where should I add the sub parser? Every argument is parsed by I have added the Arg object here: Lines 234 to 235 in 89df20f
I have added the corresponding parser action here: Lines 42 to 45 in 89df20f
I'm getting the error that -modelParams not found again. |
After a long struggle, It finally works when
I'm facing the following issue when I try to run
The same error persists when I'm switching from training the model without When I googled the error, I found out that clearing the model_dir resolves this. So, when I switched to training the model with But in this case, clearing out the model_dir will delete the training model and there wont be a trained model for running further commands. @pdxjohnny Could you please give your suggestions on this? |
Complete log for your reference:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work! Darn, tensorflow is a constant source of odd behavior. I'll look into this and get back to you about what I find this week.
You've got all the code that needs to be involved here. But I've left a few comments as to how we should re-structure things a bit.
Thank you!
Tensorflow seems to be working for me now. With regards to what I said about Similar to dffml/dffml/util/cli/parser.py Lines 95 to 103 in 062946b
Then have the Lines 91 to 98 in 062946b
Sorry if that's not clear, let me know if it's not. |
@pdxjohnny Instead of storing the model params as dict, would you like to load it as how it is done in output_specs? Could you give me the expected format in which you plan to get the params from the user and use it in the model? Also, Should I create a separate |
Hi @sudharsana-kjl. I'm going to suggest that we come at this from a different approach. (Sorry for changing things so much, you may want to follow HACKING.md/git and create a new PR, so as to not deal with merge conflicts) When you opened this pull request it was before the Data Flow Facilitator has been merged. Now that it is, I think there's a better way to pass options (not perfect, but hopefully better) Looking at Lines 176 to 193 in bf3493e
It calls For instance, tensorlfow DNN is registered in it's setup.py file: dffml/model/tensorflow/setup.py Lines 54 to 58 in bf3493e
You'd want to copy the Lines 17 to 25 in bf3493e
Lines 658 to 678 in bf3493e
|
@pdxjohnny Please review and let me know your suggestions on this. Also, could you tell me how I should approach to fix the build fail? I see that the problem is in test_model but its inside |
@pdxjohnny fixed the build error, it was a simple code change. Sorry, I didn't look at it properly last time. Please review the code changes and let me know if this is alright. |
Hi @sudharsana-kjl, thanks for your work on this. However, as I've been going through #46 to do the prep for your sources proposal I found that I had made some incorrect assumptions about |
Thanks @pdxjohnny ! |
Closed via #71 |
Implement a way to pass model specific config options