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

TODO's before queries can be used in the Discord bot #109

Closed
9 tasks done
t1-tracey opened this issue Feb 16, 2020 · 29 comments
Closed
9 tasks done

TODO's before queries can be used in the Discord bot #109

t1-tracey opened this issue Feb 16, 2020 · 29 comments
Labels
Bot Change required for the bot

Comments

@t1-tracey
Copy link
Member

t1-tracey commented Feb 16, 2020

  • address TODOs listed in TODOs for Query, a new class for bot interaction #104

  • rewrite the methods from responder.py into communicator.py

  • rewrite the instructions. Maybe this should be done by parser??

  • write the music sheet maker's questions as queries, in the create_queries() method of communicator.py

  • make sure Parser works with queries

  • determine if the use of asyncio is mandatory for the command line version to work

  • In music sheet maker, decide how to return the results of queries from one method to the overlapping one (the one which called this method), by returning Reply objects or directly doing Query.reply_to

  • check if the query_stock structure is adequate

  • address TODOs left in the code as comments

about the name 'Responder' or 'Communicator', any is fine. The problem is that Responder is not centred on communicating through Query messages.

The communicator/responder is what's between Parser and the outside. Right now, I feel that the current Responder class is still way too structured for only the command line — all of the Responder.ask_to_select_mode() ask_song_key def ask_song_headers etc. methods need to be written as each queries now

@jmmelko jmmelko added the Bot Change required for the bot label Feb 16, 2020
t1-tracey added a commit that referenced this issue Feb 17, 2020
created Information class in previous commit
t1-tracey added a commit that referenced this issue Feb 17, 2020
@t1-tracey t1-tracey removed their assignment Feb 20, 2020
@t1-tracey
Copy link
Member Author

t1-tracey commented Feb 20, 2020

I’ll work on this but I can’t make any promises about how fast.

I will probably be unlikely to be able to work on it this weekend due to writing an analysis and game design document, for example :( maybe my current availability is more as editor rather than core programmer right now

@jmmelko
Copy link
Collaborator

jmmelko commented Feb 20, 2020

Ok.
I would have liked to help, but you lost me after creating music_sheet_maker, it’s too much embedding for me!
That-Sky-Bot->Music_Cog->MusicSheetMaker->Communicator->Query

If I understand well, MusicSheetMaker will do what Responder did?
And the old code of Responder will disappear, or maybe be recycled in command_line?

@t1-tracey
Copy link
Member Author

t1-tracey commented Feb 20, 2020 via email

@jmmelko
Copy link
Collaborator

jmmelko commented Feb 20, 2020

Ok, so, to resume:

  • communication.py is a low-level module for classes such as Query, Reply, and QueryMemory
  • communicator is a personae, the only one allowed to manipulate objects in communication.py. Or, more precisely, it is the part of a personae that is dedicated to communication with the outside. It will create Queries based on what is required by Sky Sheet Music Maker. For instance it will use .yaml files.
  • Sky Sheet Music Maker is our program as a whole: it will create Songs, call Renderer, Parser, etc. However, with respect to the Cog/player, it will be quite transparent. I mean that, if the player asks a question, Maker will merely pass it to Communicator.
    So we can think of Maker as a personae, with the ability to work with his hands (Parser and Render Songs), and to use its brain, mouth and ears to communicate with the bot (Communicator).
  • Music Cog is the cog within that-sky-bot. It is the house in which dwells the Maker.

We can consider that music cog will ultimately be Alex’s responsibility.
And that the “refactoring before production” he talked about would apply to the Cog. But not to the Maker.

@jmmelko
Copy link
Collaborator

jmmelko commented Feb 20, 2020

Something like that:

B6791330-8ACE-43B9-AA73-34FD4C607CF4

@jmmelko
Copy link
Collaborator

jmmelko commented Feb 21, 2020

My drawing implies that:

  • the bot will not see Communicator, only the Maker
  • starting from an order from the bot (“create a song”), the Communicator will create the appropriate Queries (“what are the instructions for this? Which musical formats are supported?”), to which the Maker will try to Reply. Conversely, the Communicator will issue Queries (“what are the notes?”, “what is your notation?”), that the Maker will transmit back to the bot, waiting for an answer from the player.
  • the maker will parse the Replies and send the information back to the bot/cog

Another solution, which contradicts my drawing, would be to force the bot/cog to use the Query/Reply structure directly. This is what I intended in the first place. Either by forcing it to instantiate Query and Reply directly from the communication module, or by instantiating it’s own Communicator, who will handle queries.
After all, if we think of the Communicator as the part of the brain dedicated to communication, then everybody (bot, maker) should have a Communicator inside them!

So the question to be answered is:
Will Communicator() and/or communication.py be made available to the Bot/Cog, or be internal classes to the Maker?

In any case... while on one hand we know which questions the Maker can ask the player, on the other we have to list all the possible questions the player can ask to the Maker.

For questions that are not anticipated, we have 2 solutions:

  • do nothing (raise an error or send back a generic “I don’t understand” message)
  • try to guess what is meant, ie scan the question text for keywords and try to find the appropriate method. The problem is that this task is specifically the bot’s job. So I would advise against doing it. The bot/cog are supposed to send straight, explicit orders, ie call functions by their names. All the pseudo-intelligence (ie conversational abilities) is supposed to be managed by the bot.

@jmmelko
Copy link
Collaborator

jmmelko commented Feb 21, 2020

Alternate solution:

285AD456-ED31-43C4-A713-BA70DEB5304C

@t1-tracey
Copy link
Member Author

t1-tracey commented Feb 21, 2020

Ohh initially I intended for the queries, replies (and other messages e.g. Information) to be between Maker and the Bot, like your second digram and in this description^^

Another solution, which contradicts my drawing, would be to force the bot/cog to use the Query/Reply structure directly. This is what I intended in the first place. Either by forcing it to instantiate Query and Reply directly from the communication module, or by instantiating it’s own Communicator, who will handle queries.

Hehe you drew pictures to visualise it~ I like that.

I still am really unsure on how to best structure the relationship between the Maker and the Bot though (e.g. where to put Parser and Communicator?) feel free to decide on that, whichever you feel is best from your diagrams.

@t1-tracey
Copy link
Member Author

t1-tracey commented Feb 21, 2020

About Alex being in charge of the Music Cog, I think that’s fair and that’s what he intended too actually ^^ before I tried the prototype to test it. The thing is, we will need to provide very clear and concise documentation. On how to start a song, parse notes, pass on questions about key etc. (which is being done through queries now)

I’m not going to just put the burden to you btw – I intend to stay on this project, I just can’t work on it as often 😭

@t1-tracey
Copy link
Member Author

Hm I guess I just feel guilty about not being able to work on this.

I did my own version of diagram:

IMG_3406

IMG_3407

@jmmelko
Copy link
Collaborator

jmmelko commented Feb 21, 2020

Hm I guess I just feel guilty about not being able to work on this.

Your school project goes first.

Even though it may be enjoyable to work on two projects at once, to get some fresh air for your mind. You know, like a musician being in two music bands at the same time. For this reason only though! And remember that the second one is called “side project”.

I did my own version of diagram:

That’s great because I started to write down a few lines of code and I naturally converged to the first solution, and then I realized that it would be silly to invent such an elaborate communication system if it were to use it internally only.

So, a more symmetric system like the one you drew is better.

But then we’ll have to make communicator as generic as possible, since it will be used by both parties.

Actually, Communicator could be on the Maker side only, as a shortcut to generate queries.
Indeed, we can assume that the bot/cog is a natural communicator able to handle queries natively. It could use a communicator if more convenient, but could also bypass it. (In the latter case the bot/cog shouldn’t forget to instantiate a Memory object also).

jmmelko added a commit that referenced this issue Feb 21, 2020
* Added a receive method to Communicator receive a Query
* Added recipient.receive(self) to the send method of Query, to make the recipient receive the Query. Indeed, not forcing him to do so would require a completely different coding mechanism (using triggers, timeouts, etc, with the recipient periodically checking for any pending query)
* Used the \_\_getattr\_\_ attribute to pass unknown methods to communicator. This way, a sender or recipient can handle queries in a transparent way (while in practice it passes them to its communicator, exactly as a person woul process a query through its brain)
* Added recipient, owner attributes whenever needed, more to come; indeed the most important thing when handling question/answers is to know who speaks and to whom
* changed a few method names
* added arguments to the check_sender and check_recipient methods in Query so we can check the names against a list passed as an argument (useful to test if a Query is indeed addressed to you)
@jmmelko
Copy link
Collaborator

jmmelko commented Feb 22, 2020

mmmhhh...
I took a look at your music cog again and I wonder if I didn’t pushed things too far by my lack of knowledge. Everything is in it already.

For instance: song = await Questions.ask_text(self.bot, channel, ctx.author....

is nothing more than our Query() object!

And: await channel.send(

is nothing more than Information()!

And:

await channel.send(files=my_files)

looks like a Reply!

So the only thing to do is to feed these asynchronous routines (utils.Question, channel.send) content from our Query, Reply, and Information objects.

@jmmelko
Copy link
Collaborator

jmmelko commented Feb 22, 2020

Like this:

F375346C-4646-4D46-9CD7-532194B1AFC6

Q: do we need 2 communicator classes or only one will do?
(Communicator will also be a translator.)

@t1-tracey
Copy link
Member Author

t1-tracey commented Feb 22, 2020 via email

@t1-tracey
Copy link
Member Author

Oh I’m not sure, just logged onto website now to see comment (saw picture from email). Yeah I don’t know how to implement it, that was the system I had in mind though :D

Haha I will focus on school project for now as you said, one document is approaching finished state now - but I do enjoy working on side projects though.

@t1-tracey
Copy link
Member Author

Ok I think we are able to write a communicator class for the bot too (It's possible to make classes at the top of the Music Cog) so 2 communicator classes is possible

jmmelko added a commit that referenced this issue Feb 22, 2020
I have lost 1 hour because the self.queries property of QueryMemory was a class instance instead of an object instance. Because of that, the memory was shared between different instances of communicators! To solve that, I explicitely written that self.queries = []
jmmelko added a commit that referenced this issue Feb 23, 2020
Sometimes I wonder if I have not created a monster. Or, as we say in
french, a hammer to kill a fly.
@t1-tracey
Copy link
Member Author

I'm going to get back to working on this next weekend, as a heads up 👍

jmmelko added a commit that referenced this issue Feb 25, 2020
Many improvements in communication.Memory to find queries.
jmmelko added a commit that referenced this issue Feb 25, 2020
I have to better handle recalling of Informatiom objects
@jmmelko
Copy link
Collaborator

jmmelko commented Feb 25, 2020

I'm going to get back to working on this next weekend, as a heads up 👍

Great !

Maybe should we organize a live chat so I can better answer your question on how things work so far. I am available between 7:00 AM and 9:00AM GMT on mon-rue, 6:00 AM and 8:00 AM (GMT) on wed-fri, and between 6:00 AM and 6:00 PM (GMT) on weekends.

There are two things that bother me very much:

1/ I have the feeling that we have created something too complex. I am starting to wonder if what Alex meant by “make the question a model” was not simply the equivalent of utils.Question.
But the guy is very conceptual in the way he talks, hasn’t been very directive (which is good, but not good), and hasn’t been very clear about whether it was mandatory to make the changes before we can push the Cog to the Bot.

2/ the conversation (ie exchange of Queries) between bot and maker is artificial so far, since I am doing tests on a single command line program. Not only both parties are on the same machine, but the conversation is entirely sequential. Meaning that each party, after creating a Query (Query.send()), must call a method in the other party communicator (maker.communicator.receive) to force it to read the Query.

The only other way around would be to use asynchronous programming (asyncio module). In this case, one party would sequentially emit queries, and store them inside a shared global variable, or maybe inside the other party. Let’s call this variable the mailbox. In the meantime, independently, the other party would periodically check its mailbox from time to time. And vice-versa.

We could do these asyncio tests on the same machine. Actually I made a few tests but quickly encountered a new problem: prompt functions such as input() are not really compatible with asyncio, meaning that they halt the program.

Anyway, these tests would make much more sense if the asynchronous communication took place between distant machines, between server (discord/bot/music-cog) and client (player/discord).

@t1-tracey
Copy link
Member Author

Oh yes! How about a live chat on discord? I will be available on one of the weekend days. Hm ok 6am GMT is currently 5 pm here, how about 9am GMT on Saturday?

I see about asyncio, I’ve never used it outside this before (my first encounter was in the music cog) it seemed frightening to me too haha^^

The only other way around would be to use asynchronous programming (asyncio module). In this case, one party would sequentially emit queries, and store them inside a shared global variable, or maybe inside the other party. Let’s call this variable the mailbox. In the meantime, independently, the other party would periodically check its mailbox from time to time. And vice-versa.

I understand this description though!

@jmmelko
Copy link
Collaborator

jmmelko commented Feb 26, 2020

Oh yes! How about a live chat on discord? I will be available on one of the weekend days. Hm ok 6am GMT is currently 5 pm here, how about 9am GMT on Saturday?

Ok, I’ll send you a message the day before to confirm.

@t1-tracey
Copy link
Member Author

t1-tracey commented Feb 27, 2020 via email

@jmmelko
Copy link
Collaborator

jmmelko commented Feb 27, 2020

Sure^^ 👍 it’s live text chat, to confirm?

Yes, we could use the chat channel of your discord Sky Music Maker account, or another temporary channel.
Discord is more or less live, isn’t it?
We could invite Strawb also, I wonder why she hasn’t given any news recently

@jmmelko
Copy link
Collaborator

jmmelko commented Feb 28, 2020

That’s ok for Saturday at 9:00 GMT = 10:00 Paris = 20:00 Melbourne.
(We can still shift by +/- 1h if you want, in this case tell me asap)

@t1-tracey
Copy link
Member Author

t1-tracey commented Feb 28, 2020

Yep any channel is fine, sorry I didn’t get to respond yet^^
And 20:00 Melbourne is ok with me 👍
And yeah we can invite Strawb 😊

@jmmelko
Copy link
Collaborator

jmmelko commented Mar 1, 2020

It was nice talking to you. Sorry I talked too much but I felt the need to report to you on what I did.

In the meantime I have added a working script using asyncio. It does not use our program yet.

@t1-tracey
Copy link
Member Author

t1-tracey commented Mar 1, 2020 via email

@t1-tracey
Copy link
Member Author

ok I managed to run through CommandLinePlayer and MusicSheetMaker, and I think I'm starting to understand the changes better now^^
once we are clearer on how to use asyncio, I'd like to help with creating the questions.

some of the questions, such as ask for input mode, rely on information after Parser has parsed the notes from the user

@jmmelko
Copy link
Collaborator

jmmelko commented Mar 2, 2020

once we are clearer on how to use asyncio, I'd like to help with creating the questions.

Actually we could use asyncio without changing anything else, just by adding the async keyword before definitions and the await command before calls 😝. People sometimes do that to speed up execution (independent tasks are run concurrently) , but in our case we don’t have any time issue related to queries.

So, the real question behind asyncio the question will be to use a mailbox concept instead of a puppet concept. I mean, instead of having two puppets pulling strings from each other (receive(), execute_queries()), they would be periodically checking their inbox for new messages to process.

I have asked info to Alex about the urge to use asyncio.

some of the questions, such as ask for input mode, rely on information after Parser has parsed the notes from the user

Yes.
We could arrange for these questions to be answered before or after parsing though. (if answered_before then dont_ask).

@t1-tracey
Copy link
Member Author

t1-tracey commented Mar 2, 2020

Ah okay, I see

Oh yeah but the input modes and song key need to be asked after parsing, e.g. —

Choose song key from A, D, E

A, D, E are calculated from the possible keys method determined by Parser.

And:

Multiple possible input modes detected:
1) Sky keyboard
2) English notation
Choose input mode: (1-2)

The two options were determined by Parser, after the user has typed the notes.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bot Change required for the bot
Projects
None yet
Development

No branches or pull requests

2 participants